Code development platform for open source projects from the European Union institutions

Skip to content
Snippets Groups Projects

[OEL-1255] OEL-1281: Upgrade path from oe_bootstrap_theme_paragraphs

Merged Francesco SARDARA requested to merge OEL-1281 into EPIC-OEL-1255-migrate-paragraphs

Created by: donquixote

OEL-1281

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit 28127ffd
  • 168 // Remember that the revision needs saving.
    169 $modified = TRUE;
    170 }
    171 if (!$modified) {
    172 // No saving is needed.
    173 continue;
    174 }
    175 $paragraph_revision->setNewRevision(FALSE);
    176 $paragraph_revision->save();
    177 }
    178 }
    179
    180 /**
    181 * Removes legacy field instances from oe_bootstrap_theme_paragraphs module.
    182 *
    183 * @param string[][] $field_names_by_bundle
  • Francesco SARDARA
  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit 28127ffd
  • 114 foreach (array_intersect_key(
    115 $fields_map[$dest_field_name]['bundles'],
    116 $fields_map[$source_field_name]['bundles'],
    117 ) as $bundle) {
    118 $field_names_by_bundle[$bundle][$dest_field_name] = $source_field_name;
    119 }
    120 }
    121
    122 return $field_names_by_bundle;
    123 }
    124
    125 /**
    126 * Migrates field data from the old oe_bootstrap_theme_paragraphs module.
    127 *
    128 * Normally this should happen through a batch process, e.g. via $sandbox in a
    129 * hook_update_N(). Unfortunately, hook_install() does not support batch
    • Created by: drishu

      why not use queue API?

    • Created by: donquixote

      I think the main reason for batch in hook_update_N() is to work around max_execution_time, and also to show a progress bar when doing it through the UI. For memory concerns, we simply have to avoid keeping all data in memory at the same time, so that garbage collection can do its job. I think the max_execution_time mostly applies in web UI, for cli it is typically disabled.

      In hook_install() we don't have the luxury of splitting up the current process, because the calling code expects everything to finish in one call. Using queue, afaik, would mean that part of this operation would happen during cron after the install. So it would depend on people actually running cron, and operations directly after the install could not rely on the operation being complete.

    • Created by: drishu

      I've tested and this brings no benefit when calling from install, ignore pls

  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit 28127ffd
  • 179 'oe_bt_links_block_background' => TRUE,
    180 ],
    181 ];
    182
    183 $actual_updated = [];
    184 $actual_deleted = [];
    185 foreach ($ids as $name => $id) {
    186 $updated_paragraph = Paragraph::load($id);
    187 self::assertNotNull($updated_paragraph);
    188 foreach ($expected_created[$name] as $field_name => $value) {
    189 if (!$updated_paragraph->hasField($field_name)) {
    190 continue;
    191 }
    192 $actual_updated[$name][$field_name] = $updated_paragraph->get($field_name)->value;
    193 }
    194 foreach ($expected_deleted[$name] as $field_name => $deleted) {
    • Created by: drishu

      I know this is constructed using $paragraphs_data which we control, but its confusing and prone to human error, a simple listing of checks one after the other would have been simple and clear. I prefer SOLID and KISS programming principles. This time we keep it like this , but please read up, on above, especially on "KISS"

    • Created by: donquixote

      The purpose of this data collection is to have a more useful and more complete diff output on failure. But yes, with individual assertions the code would look simpler.

  • Created by: drishu

    Review: Approved

  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit 8a4543df
  • 95 ];
    96
    97 /**
    98 * @var string[][] $field_names_by_bundle
    99 * Legacy field names, indexed by paragraph type and destination field name.
    100 */
    101 $field_names_by_bundle = [];
    102 foreach ($field_names_map as $dest_field_name => $source_field_name) {
    103 if (!isset($fields_map[$source_field_name])) {
    104 // No field to migrate from.
    105 continue;
    106 }
    107 if (!isset($fields_map[$dest_field_name])) {
    108 // No target field to migrate to - this is unexpected.
    109 continue;
    110 }
    • Created by: brummbar

      This cannot be true, as we just imported the field configurations during install. So either we drop this, or we throw an exception.

    • Created by: donquixote

      Actually it might be true, if paragraph types have been removed on the target system. But this would have blown up earlier I suppose. Either way, let's consider this case not supported.

  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Created by: brummbar

    Review: Changes requested

    Tested, works good, I did a local test with around 20 paragraphs and I felt the speed was already impacted, but the code has nothing that can be improved on. We might have to ask UCPKN to run this on their database to see if it's too slow already.

  • Created by: brummbar

    Review: Approved

  • Merged by: donquixote at 2022-03-30 17:56:28 UTC

  • merged manually

  • Please register or sign in to reply
    Loading