🇳🇮Nicaragua @dinarcon

benjifisher → credited dinarcon → .

mikelutz → credited dinarcon → .

dinarcon → created an issue.

Committed.

dinarcon → created an issue.

Thanks for the quick reply. Committing directly commit to 7.2.x is fine. I am not sure what happened with the issue fork. When I created my PR it was against the 7.x-2.x branch. I am surprised that it had a merge conflict because my commit changed only one line compared to HEAD. The MR now says that it is against 4.x with a lot more changes. I don't really understand what happened there.

BTW, sorry for changing the visibility of the `3450182-projects-moved-into` branch. I was trying to review things and clicked the wrong link. I changed it back to visible.

dinarcon → changed the visibility of the branch 3450182-projects-moved-into to active.

dinarcon → changed the visibility of the branch 3450182-projects-moved-into to hidden.

Added a MR with the suggested change.

dinarcon → created an issue.

ricardoamaro → credited dinarcon → .

This is still an issue. Version 4.0.3 contains core_version_requirement: ^9 || ^10 in the radioactivity.info.yml file, but the issue reported here is not being able to add the module via Composer. That is because 4.0.3 does not declare Drupal 10 compatibility in the composer.json file. See https://git.drupalcode.org/project/radioactivity/-/blob/4.0.3/composer.j...

This has already been fixed in https://git.drupalcode.org/project/radioactivity/-/commit/5b0200bc08ca66..., but that commit was not part of the 4.0.3 release.

Doing what is described in #6 or #7 allows the module to be installed. Tagging a new release would be very much appreciated.

dinarcon → made their first commit to this issue’s fork.

Thanks for stepping up @RobLoach I have added you as a maintainer of this project https://www.drupal.org/project/ckeditor_a11ychecker → and also of https://www.drupal.org/project/ckeditor_balloonpanel → as it is a dependency.

benjifisher → credited dinarcon → .

benjifisher → credited dinarcon → .

volkswagenchick → credited dinarcon → .

volkswagenchick → credited dinarcon → .

benjifisher → credited dinarcon → .

benjifisher → credited dinarcon → .

We might need the to review the skip logic in the view_mode and bundle process pipelines. Let's see what the test bot reports about the patch first.

Please review.

dinarcon → created an issue.

Please review patch. While saving a message is different than writing to the map are different things, we might consider doing both. See 🐛 Paragraph Migrations broken by archived flag RTBC

In case I am not able to join the next meeting in real time, I would like to propose we add a thread to talk about about this issue 🐛 Migrations fail due to missing dependency when dependency has skipped rows by the source plugin Needs review

I have updated the issue summary. Also changing state to Needs review for feedback on the proposed patch. No interdiff against previous patches as the approach is somewhat different.

This is affecting 🐛 Paragraph Migrations broken by archived flag RTBC

As mentioned in the issue summary, this is caused by #2797505-74: Migrations fail due to missing dependency when dependency has skipped rows by the source plugin → . When that issue is fixed, we might not have to do anything. In the meantime, I propose to save to the map when a paragraph row needs to be skipped. See related issue for context and attached patch for review. No interdiff as the approach is somewhat different.

I have reviewed patches in commets #61 and #70.

If I understand things properly, #61 would be an API break. Per \Drupal\migrate\Plugin\MigrateSourceInterface::prepareRow, we allow returning FALSE to signal the row should be skipped. I tested this on a recent project. The error about skipped rows in dependent disappears, but that is because the rows where imported when they should have not. I do not have a Core example handy, but the ParagraphsItem source plugin relies on skipping on FALSE to intentionally prevent migrating paragraph entries that are no longer in use.

Patch #70 would mark the row as skipped effectively solving the issue. In this patch, this is a byproduct of calling throw new MigrateSkipRowException('Row skipped by source plugin.');. That is, MigrateSkipRowException signals that the row should be saved to the map. The actual saving is performed by MigrateExecutable in the import method. Namely, in this block of code:

        try {
          foreach ($pipeline as $destination_property_name => $plugins) {
            $this->processPipeline($row, $destination_property_name, $plugins, NULL);
          }
          $save = TRUE;
        }
        catch (MigrateException $e) {
          ...
        }
        catch (MigrateSkipRowException $e) {
          if ($e->getSaveToMap()) {
            $id_map->saveIdMapping($row, [], MigrateIdMapInterface::STATUS_IGNORED);
          }
          ...
        }

Throwing the exception probably suffices. I do not understand why the proposed changes to \Drupal\migrate\MigrateExecutable::processPipeline and \Drupal\migrate\Row are necessary. I propose a slightly different approach. Save to the map directly in \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::next Optionally, we can also include a message there. I have attached a patch with both, but only is saving to the map is strictly necessary.

This topic was discussed in this Slack thread some time ago. For references, copying part of the conversation below:

So, prepareRow row in my custom source plugin returned FALSE and the records was skipped. But, it failed to do something that \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::prepareRow does. In the method implementation for SourcePluginBase , which is where the migrate_prepare_row and migrate_MIGRATION_ID_prepare_row hooks are fired. If a record is marked to be skipped, it is saved to the idMap.

While there is a boolean to determine if the record should be saved or not ($save_to_map), saving to the map makes sure the record is accounted for when the migrate API checks if all records have been processed for a migration. This check is also performed in \Drupal\migrate\Plugin\Migration::checkRequirements So, if a migration dependency is set as required, it has to have all rows processed. A tangential note, but worth mentioning.

So, the solution was manually saving to the idMap in my custom source process' prepareRow method. This fixed the problem for me.

Writing to the idMap is accounted for by \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::prepareRow when the result comes from an implementation of the migrate_prepare_row and migrate_MIGRATION_ID_prepare_row hooks. But if you implement the method in your source plugin, you need to write to the map yourself.

The original issue asks for improving the documentation \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::prepareRow If we make this change in \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::next we keep BC. If we only improve the API, we would have to rely on developers to manually save to the map if they implement \Drupal\migrate\Plugin\MigrateSourceInterface::prepareRow in their custom source plugins.

dinarcon → created an issue.

Allow active field to be sorted and filtered. Add no result behavior.

The attached patch adds a view to allow sorting and filtering of site alerts. Please review.

dinarcon → created an issue.

This patch makes the element's form_key lowercase in the `d7_webform_submission` source plugin. Please review.

dinarcon → created an issue.

Webform for D10 is still in beta. You might need to update your platform requirements in the root's composer.json file.

composer config minimum-stability beta
composer require 'drupal/webform_migrate:2.0.x-dev@dev'

Attaching documentation patch for review. Shall we also mention this in the README.md file?

dinarcon → created an issue.

smustgrave → credited dinarcon → .

hestenet → credited dinarcon → .

benjifisher → credited dinarcon → .

Thanks for working on this!

I applied the patch and got the migrate_tools versions of the commands while using Druah 12. Per https://www.drush.org/12.x/commands/#attributes-or-annotations it says either PHP8 Attributes or Annotations are valid ways to declare a command. The current patch adds PHP8 Attributes on top of the existing Annotations. Shall we remove the latter?

For the record, I tested removing the PHP8 Attributes and the commands indeed worked with just the annotations. I guess my questions is if migrate_tools should try to match the PHP requirements from the Drush versions that it supports. Per https://www.drush.org/12.x/install/#drupal-compatibility, Drush 12 requires PHP 8.1. If we keep the annotation version only, we can support Drush 11 and 12 at the same time without requiring PHP 8.1, right?

Awesome that this has landed. Thanks everyone!

@bnjmnm mentions 10.2 in the message where the issue status is changed to fixed. Does this needs to be backported to Drupal 10? Or is it meant to be a feature only available in Drupal 11?

Thanks for the bug report @arti_parmar. This patch should fix the issue.

volkswagenchick → credited dinarcon → .

benjifisher → credited dinarcon → .

DamienMcKenna → credited dinarcon → .

benjifisher → credited dinarcon → .

volkswagenchick → credited dinarcon → .

Attached is a field with the proposed changed.

dinarcon → created an issue.