🇺🇸United States @benjifisher

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.

@daffie:

Even better: you removed the condition group completely. Good idea, I wish I had thought of it.

The code looks great. I have resolved all the threads on the MR.

Testing

I hacked core/modules/node/migrations/d7_node_complete.yml to generate some messages:

title:
    - plugin: log
      source: title

I have a D7 site that I use for migration testing, with 100 nodes from devel_generate. On my D11 site, I enabled migrate_drupal_ui and imported the content from that site.

Then another hack: I used SQL queries to change the level of several messages (10 each for level 2, 3, 4).

After those hacks, I tested the filters: filtering on one or two levels (severity), filtering on "qu" in the message, filtering on both the level and the message. Everything worked as expected.

I forgot to say: thank you, @orbmantell, for fixing this!

The change is simple, and it does the right thing. +1

My team runs acsf-init-verify on every git push. (I am not sure why.) So I run into this deprecation notice a lot. After applying the patch, the warnings go away. +1

I made 3 more comments on the MR (and resolved all the existing threads). Two are minor, but using an AND condition group instead of an OR condition group is important.

I also considered whether the user input is handled securely. It is: all session data ends up inside $condition_or->condition(...), so it should be safe.

I still need to do some manual testing once the AND condition is restored.

@daffie:

I am sorry to be so slow in responding.

It is kind of crazy, but true: as I said on the MR, "you almost never have to pass an object by reference." It is pretty well explained in the PHP manual. From the Introduction to Classes and Objects page (https://www.php.net/manual/en/oop5.intro.php):

PHP treats objects in the same way as references or handles, meaning that each variable contains an object reference rather than a copy of the entire object. See Objects and References

Follow that link to see an explanation and some examples.

I will have a look now at the updates to the MR.

I am adding issue credit for the attendees.

benjifisher → created an issue.

benjifisher → created an issue.

poker10 → credited benjifisher → .

poker10 → credited benjifisher → .

poker10 → credited benjifisher → .

poker10 → credited benjifisher → .

greggles → credited benjifisher → .

benjifisher → created an issue.

I created this issue targeting the 2.1.x branch, but it should be changed to the 3.0.x branch when that is available.

benjifisher → created an issue.

benjifisher → created an issue.

benjifisher → created an issue.

I rebased the MR. The only conflict was from 📌 Fix DrupalPractice.Objects.GlobalFunction in hooks Postponed , which replaced t() with $this->t().

benjifisher → created an issue.

benjifisher → created an issue.

benjifisher → created an issue.

I notice that the current MR does not change the performance test. I notice that 📌 Remove cache tag checksum assertions from performance tests Active is now RTBC, so maybe we will not run into that problem in the future.

I am not sure how useful this change would be. I guess that @joachim ran into a situation where it would help. The code change is pretty simple, so I do not object to making the change.

@joachim, can you share the process pipeline you were debugging when you created this issue? It would help to provide context.

Since the process pipeline is an array, maybe it would be more suggestive to use brackets instead of parentheses. For example, using a line from one of the tests in the MR:

"d7_field:type:process_field(1): Can't migrate field 'field_event' with 'todate' settings. Enable the datetime_range module. See https://www.drupal.org/docs/8/upgrade/known-issues-when-upgrading-from-drupal-6-or-7-to-drupal-8#datetime"

What do you think of this instead?

"d7_field:type:process_field[1]: Can't migrate field 'field_event' with 'todate' settings. Enable the datetime_range module. See https://www.drupal.org/docs/8/upgrade/known-issues-when-upgrading-from-drupal-6-or-7-to-drupal-8#datetime"

I also made two small suggestions on the MR.