🇺🇸United States @benjifisher

benjifisher → created an issue.

benjifisher → created an issue.

In the migrate_plus module, I credited StackOverflow: https://git.drupalcode.org/project/migrate_plus/-/blob/6.0.x/src/Plugin/...

    // @see https://stackoverflow.com/a/47718734/3130080
    return array_map(NULL, ...$table);

Does that seem problematic? Once you know to look for it, that usage of array_map() is also documented in the PHP docs: https://www.php.net/array_map#example-5199. Unlike the code in this issue, it is a one-liner.

#3506605 also adds a new source plugin. I forgot to remove source_module from that plugin during the rebase, but PHPStan reminded me.

I just rebased the MR on the current 11.x. There were merge conflicts from 📌 Move the d8_config source plugin to the migrate module Active , and I also checked 📌 Convert recently added test plugin to use attributes Active .

Details: #3506605 deprecates the d8_config source plugin in the migrate_drupal module. Part of that deprecation is overriding the constructor from the parent class and triggering a deprecation warning. Before that issue, the class did not override the constructor, so it did not need use statements for the constructor's arguments. The only merge conflicts were in the use statements at the top of the file.

I rewrote the change record.

I will also update 📌 Convert MigrateSource plugin discovery to attributes Active , the change record for 📌 Convert MigrateSource plugin discovery to attributes Active .

benjifisher → created an issue.

I think the only place in the migrate module, outside of tests, where requirements_met refers to the source plugin (not the destination plugin) is SqlBase::checkRequirements().

I added 📌 Remove mimimum_version and requirements_met from MigrateSource attribute class Active , so I can cross off another one of the remaining tasks.

It is pretty clear that requirements_met is used only in the migrate_drupal module:

$ grep -ril minimum_version core/modules/mig*
core/modules/migrate/src/Attribute/MigrateSource.php
core/modules/migrate/src/Annotation/MigrateSource.php
core/modules/migrate_drupal/tests/src/Unit/source/DrupalSqlBaseTest.php
core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php

The situation is less clear for requirements_met:

$ grep -ril requirements_met core/modules/mig*
core/modules/migrate/tests/modules/migrate_events_test/src/Plugin/migrate/destination/DummyDestination.php
core/modules/migrate/tests/modules/migrate_missing_database_test/src/Plugin/migrate/source/MigrateMissingDatabaseSource.php
core/modules/migrate/tests/src/Kernel/SqlBaseTest.php
core/modules/migrate/src/Plugin/Derivative/MigrateEntityRevision.php
core/modules/migrate/src/Plugin/Derivative/MigrateEntityComplete.php
core/modules/migrate/src/Plugin/Derivative/MigrateEntity.php
core/modules/migrate/src/Plugin/migrate/destination/NullDestination.php
core/modules/migrate/src/Plugin/migrate/destination/DestinationBase.php
core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
core/modules/migrate/src/Attribute/MigrateDestination.php
core/modules/migrate/src/Attribute/MigrateSource.php
core/modules/migrate/src/Annotation/MigrateDestination.php
core/modules/migrate/src/Annotation/MigrateSource.php
core/modules/migrate_drupal/tests/src/Unit/source/DrupalSqlBaseTest.php
core/modules/migrate_drupal/tests/src/Kernel/d7/FieldDiscoveryTest.php
core/modules/migrate_drupal/tests/src/Kernel/d6/FieldDiscoveryTest.php
core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php

Many (most? all?) of those uses refer to requirements_met in the MigrateDestination annotation and attribute classes. The attribute class is already part of Drupal 11.1.0, so it is too late to remove properties from that class without first deprecating them. But I think we can remove requirements_met from the attribute class.

benjifisher → created an issue.

I am adding a detailed list of changes to the issue summary.

While I was preparing this summary, I noticed that a test had an @group migrate_drupal_ui annotation even though it was in the migrate module and this issue moves it to the migrate_drupal module. I changed it to @group migrate_drupal.

We did not have enough participants for a meeting, so we skipped this week.

We discussed this issue briefly at 📌 Drupal Usability Meeting 2025-04-11 Active . That issue will have a link to a recording of the meeting. We were primarily discussing the related issue #29338: Hide Promoted/Sticky fields by default in Form display → .

We agree that this is a good idea. Thanks for working on this issue!

For the record, the attendees at the usability meeting were benjifisher, rkoller, and simohell.

I added a comment to 📌 Clarify/replace 'stage' terminology in Package Manager Active .

We discussed this issue at 📌 Drupal Usability Meeting 2025-04-11 Active . That issue will have a link to a recording of the meeting.

For the record, the attendees at the usability meeting were benjifisher, rkoller, and simohell.

We are not familiar with the code base and the various ways that "stage" is used, but we tried to suggest some guidelines for naming things.

First, we agree with the first suggestion in the Proposed Resolution. If adding "directory" is too long, then use "staging" or "staged", not "stage" in the context of the staging directory. For example: use StagingBase or StagingDirectoryBase instead of StageBase.

Second, use "phase" or "step" instead of "stage" when you can. Use "step" if it is possible to move back, or "phase" if only one direction is possible.

I hope those suggestions are helpful!

@catch, @quietone: Thanks for the prompt replies!

Earlier, I added this question to the Remaining Tasks:

Decide when to issue a deprecation message because source_module is present, but should not be, See Comments #50, #56, #77.

I think the current version of the MR gets it right: only issue a deprecation if source_module is present AND migration_tags include neither "Drupal 6" nor "Drupal 7" AND migrate_drupal is not one of the providers.

Here is why that is the right decision. I ran into some failing tests because there are 4 migrations in the block_content module that use the embedded_data source plugin and have the source_module property. At first, I deleted source_property, but then the ValidateMigrationStateTest tests failed. So we need to allow source_module if there is an appropriate tag.

As a bonus, I finally found the right way to fix the deprecations: make sure that config is installed in MigrationPluginListTest::testGetDefinitions() so that MigrationPluginManager::getEnforcedSourceModuleTags() (the migrate_plus version) works.

Even though this issue still needs an update to the summary and the change record, I am going to say it is ready for review.

I am adding a few items to the "Remaining Tasks" in the issue summary.

I am mostly reverting the source plugins to be the same as in the 11.1.x branch, but I have to be careful not to revert the changes from 🐛 Move content_translation I18nQueryTrait to migrate module Active and another issue or two.

This reverts what I said in the second half of Comment #66 along with a lot of what we did in 📌 Convert MigrateSource plugin discovery to attributes Active .

As long as we are removing source_module from the MigrateSource attribute class, should we also get rid of these?

   * @param bool $requirements_met
   *   (optional) Whether requirements are met. Defaults to true. The source
   *   plugin itself determines how the value is used. For example, Migrate
   *   Drupal's source plugins expect source_module to be the name of a module
   *   that must be installed and enabled in the source database.
   * @param mixed $minimum_version
   *   (optional) Specifies the minimum version of the source provider. This can
   *   be any type, and the source plugin itself determines how it is used. For
   *   example, Migrate Drupal's source plugins expect this to be an integer
   *   representing the minimum installed database schema version of the module
   *   specified by source_module.

If so, do that in this issue or in a follow-up?

benjifisher → created an issue.

benjifisher → created an issue.

We skipped the 2025-03-27 meeting because of DrupalCon Atlanta. I am repurposing this issue for today's meeting.

poker10 → credited benjifisher → .

benjifisher → created an issue.

I do not have time now to make the agreed changes, but this commit should fix some of the failing tests.

Four of the migrations in the block_content module

• Use the embedded_data source plugin;
• Have the "Drupal 6" or "Drupal 7" migration tag (in fact, the migrations have both tags);
• Include source_module in the source configuration.

Maybe I should remove the source_module from those migrations, but for now I am changing the logic so that we do not get a deprecation message for these. See Comments #50, #56.

benjifisher → created an issue.

I think Comment #73 and the third option in Comment #69 are the same thing. Let's do that. (As soon as I have a little spare time.)

For the record: another option is to move all the source plugins (except those in the migrate module) to the migrate_drupal module.

I rebased the feature branch for MR 10668, and there were no conflicts.

It is surprising how simple the change is (+33/-15) given the complexity of the discussion here. I guess this simple approach is just creating a "dumping ground" instead of a way to add first-class attributes to an annotation class. I am still working on understanding the discussion here, so maybe I do not have it quite right.

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

@godotislate:

Thanks for explaining that!

In Comment #66, I wrote:

This change breaks BC, but that is OK if we get it done before Drupal 11.2.0 is released.

The MigrateSource attribute was defined in 📌 Convert MigrateSource plugin discovery to attributes Active (I know you know, but I want a convenient link), and that was committed to the 11.x branch only (not to 11.1.x, not in any release).

I was going to say that any module that uses MigrateDrupalSource should declare a dependency on migrate_drupal, and that we should say so in the change record. Then I realized that there are a bunch of core modules that define D6/D7 sources, and those modules should not depend on migrate_drupal.

I see your point.

What options do we have?

1. Clear that apcu cache in migrate_drupal's hook_install (Comment #68).
2. Fix ✨ Allow attribute-based plugins to discover supplemental attributes from other modules Active and this issue before 11.2.0 is released.
3. Revert the part of #3421014 that converts most core source plugins to attributes, and the commit for this issue that updates those source plugins.

Any others?

I think the third option is the simplest. We discussed that option on #3421014, but I (and maybe others) did not understand the implications.

I implemented @alexpott's suggestion: trigger a deprecation warning if there is a source_module annotation on a source plugin (or in its configuration) if it is not needed. That led to a complete rewrite of Drupal\migrate_drupal\MigrationPluginManager::processDefinition().

Then I created the MigrateDrupalSource attribute class, extending MigrateSource, and moved the source_module property to the new class. And I updated most code source plugins to use the new attribute class. This change breaks BC, but that is OK if we get it done before Drupal 11.2.0 is released.

Let's see what the testbot thinks.

benjifisher → created an issue.

Based on Comment #27, the status should still be NW.

I rebased the MR. One change that got lost is replacing "Choose an option below" with "Choose an option". I think that #3386762 changed it to "Choose a field type":

I think that is good.

I did not test much, but I did look at the Reference options, and I think our changes are still there.

> ... there may be a strong need to move the existing BPMNModeler bits that are in the eca module into the BMPN.io module.

I already added 🌱 Decide what to do with the modeller_bpmn module Active to work on that question.

benjifisher → created an issue.

Depending on where the documentation goes, we might want to move this issue to the queue for the eca module.

benjifisher → created an issue.

Try to avoid confusion between the string 'TRUE' and the boolean TRUE.

Probably we should do the two steps in the reverse order of what I wrote in the issue summary. Then the pipelines will show that the static analysis does what we want and that we have caught all instances.

Can cspell help with this? I think it complains about e-mail with a hyphen; maybe we can get it to complain about modeller.

benjifisher → created an issue.

benjifisher → created an issue.

benjifisher → created an issue.

benjifisher → created an issue.

greggles → credited benjifisher → .

Now that 📌 Convert MigrateSource plugin discovery to attributes Active is fixed, we can return to this issue. In an earlier comment, I suggested that we would have to rewrite the MR for this issue, but I changed my mind. I rebased the MR, resolving conflicts between this issue and #3421014.

I think we need to update the issue summary with the current proposed resolution.

I think that Drupal\migrate_drupal\MigrationPluginManager::processDefinition() needs some further work. I have to think about whether @alexpott's comment on that issue is still valid, or if the recent changes make it obsolete.

benjifisher → created an issue.

@kristen pol:

Thanks for trying out this recipe.

I cannot reproduce the problem. This is what I tried:

1. Install Drupal from the current 11.x.
2. Install Drush and this recipe: composer require drush/drush drupal/import_csv.
3. Install Drupal with the standard profile: drush si.
4. Apply the recipe: drush recipe recipes/import_csv.
5. Log in.
6. Visit /admin/content/migrate_source_ui.
7. Select "Nodes (supports csv) from the select list (the default) and upload recipes/import_csv/examples/nodes.csv as the source.
8. Submit the form ("Migrate").

I did not get any errors, and the migration created three nodes.

On /admin/config/content/migrate_source_ui, I see the message,

The module will use Drupal's default temporary:// stream if this is not set.

My test seems to confirm that. At least, the module works without that being set.

Are you getting the current versions of the migration modules? If not, that might explain why you are getting different results.

Failed to connect to your database server.

That message comes from the migrate_drupal module. See 📌 Improve error reporting from migrate_drupal_migration_plugins_alter() Needs work . Can you uninstall migrate_drupal or supply database credentials for it? I do not think there is anything this recipe, or the modules it installs, can do to work around this core bug.

To do: Add some more issues (alpha target? beta target?)

1. Convert from annotations to attributes.
2. Use constructor promotion. (beta target)
3. Change names of class variables and methods that currently use "Eca".
4. Consistently spell modeler with one "l". (Can cspell help with this? I think it complains about e-mail with a hyphen; maybe we can get it to complain about modeller.)
5. Add documentation for updating to Modeler API. (README? Documentation on d.o?)
6. Decide what to do with the modeller_bpmn module. (Move to modeler_api module or bpmn_io module or something else? Or keep it in the eca package?)

I think the work I have done so far is enough for the current issue.

I think the work I have done so far is enough for the current issue.

On second thought, I do not think we need to move the Modellers service to the modeler_api module. It consists of convenience methods (mostly wrapping the plugin manager) and some methods closely tied to the eca module (such as events(), which returns a sorted list of all the Event plugins). We may decide to move some of that to the modeler_api module, but not as part of this issue.

Other than the service, I think the MR handles everything in the issue description. This issue is ready for review, along with the related issues for the eca and bpmn_io modules.

We spent half the meeting talking about 📌 Refine field descriptions Active . I do not think we need to add yet another comment to that issue.

We spent the rest of the meeting discussing possible follow-ups to 🐛 Better handling of timezones for relative default date + times Active , but we have not created any so far.

I think we can call this issue done even though the last "Remaining task" in the issue summary is still open.

One option is to replace the three add*() methods with the single add() method. It would take some sort of type argument (event, condition, or action for the ECA module). I am not sure how the client module and the API module would communicate about the available types.

Maybe a better model is an event system. When the model is updated (or created or deleted), the API module dispatches an event. The client module subscribes to the event, and decides how to update its config entity.

The current data model uses an Eca config entity and a Model config entity with the same ID. Also, the Model entity has an Eca entity as a class property. As we think about how to decouple from the ECA module, we might decide to change that.

The core user module provides forms (Manage Permissions) that can be used by other modules. The user modules provides a Route Subscriber, and other modules declare that route provider in the attributes for their bundle entities. I think the same method will work here.

References:

• Drupal\user\Entity\EntityPermissionsRouteProvider
• Link Templates →

benjifisher → created an issue.

benjifisher → created an issue.

I added a comment to 🌱 Providing additional methods of navigating the admin interface Active , but the main issue we discussed was 🐛 Keyboard navigation does not follow visual order of items Active .

Usability review

We discussed this issue briefly at 📌 Drupal Usability Meeting 2025-03-21 Active . That issue will have a link to a recording of the meeting.

For the record, the attendees at the usability meeting were aaronmchale, benjifisher, rkoller, simohell, and worldlinemine.

We are still "talking about talking", or trying to decide on a framework for further discussion. It is tempting to rush into suggestions, and there are several that have already been mentioned on this issue, such as

• Rearrange the menu
• Add related/deep links
• Search (like the coffee module)

But first, we need a way to focus the discussion. We need goals, criteria, and metrics. Here is a start:

1. Fewer clicks
2. Lower cognitive load
3. Card-sorting, Tree testing (UX research)
4. Allow modules to extend

@jurgenhaas: Thanks for opening the 3.0.x branch!

I am adding testing instructions to the issue summary.

@jurgenhaas: Thanks for opening the 3.0.x branch!

I am adding testing instructions to the issue summary.

I am adding testing instructions to the issue summary.

benjifisher → created an issue.

benjifisher → created an issue.

@rkoller:

On Slack, I said

My commit changes core/lib/Drupal/Core/Field/PreconfiguredFieldUiOptionsInterface.php. Just search for the classes that implement that interface and add a description key to getPreconfiguredOptions(). (Or search for that function.)

I was wrong. It is more complicated than that.

I just pushed a commit that implements the Usability team's recommendations for reference fields. I mostly followed the example of hook_field_ui_preconfigured_options_alter() in core/modules/field/field.api.php.