🇺🇸United States @benjifisher

I am giving credit to the participants listed in the Security Team's private notes.

benjifisher → created an issue.

benjifisher → created an issue.

i guess @benjifisher might be familiar with it from discussions in the security team?

I can neither confirm nor deny.

benjifisher → created an issue.

On second thought, I do not think we need to add hasEmptyDestinationProperty().

First reason: it is safe to call removeEmptyDestinationProperty() without first testing hasEmptyDestinationProperty().

Second reason: if code does need to test, it can just use in_array($field_name, $row->getEmptyDestinationProperties()).

Source and destination properties need the has... methods because they use the NestedArray methods, but $emptyDestinationProperties is just a flat list of strings.

Thanks for the feedback.

I think we should not implement the two convenience methods. There are several reasons:

The has...() convenience method calls hasDestinationProperty() and (if that returns FALSE) hasEmptyDestinationProperty(). If the code next calls the remove... convenience method, as in your snippet, it calls the same has... method(s). That seems sloppy.

The method names are long, and they do not adequately describe what the methods do. Anyone reading the code will have to look up what the method does. It is convenient to write, but not to read.

Adding extra methods means we have to add and maintain test coverage for them.

In simple cases with content entities, you can get the same effect by adding overwrite_properties to the configuration of the destination plugin. (See Drupal\migrate\Plugin\migrate\destination\EntityContentBase.) I think this covers the most common use cases. I do not want to add convenience methods to the API if they are only going to be used by occasional projects that have especially complex business logic. Those projects can add convenience functions (not methods on the Row class) themselves.

Thanks for suggesting this addition to the Migrate API. I think this is more of a feature request than a bug report, so I am updating the category.

I am trying to understand the use case for this addition. Is it mostly for migrations that update existing entities? If a migration is creating a new entity, then I think it does not matter whether the Row object has a null property or is just missing that property.

I also wonder whether we need all four of the new methods. Are hasDestinationOrEmptyProperty() and removeDestinationOrEmptyProperty() really useful? Again, what is the use case?

For both of those questions, it would help to see a code snippet of how you are using (or plan to use) the new methods.

benjifisher → created an issue.

@daffie:

The two snippets in the issue summary use $query->join('inner', ...) and $query->join('left', ...). I think that is a little odd, but it is all right since they are not explicitly described as equivalent.

But the first before/after example in the change record (CR) has the same examples. There, I think it is a problem: an inner JOIN is different from a left JOIN, isn't it?

I will not move the issue back to NW for that, but I will add an issue tag.

I have an even smaller suggestion about another example in the CR:

$condition = \Drupal::database()
  ->condition('AND')
  ->compare('d.entity_id', 'b.entity_id')
  ->compare('d.entity_type', 'b.entity_type')
  ->compare('d.field_name', 'b.field_name');

As I read it, the coding standards → encourage breaking before each method if each method belongs to the same object. Here, condition() is a method on the Connection object, and the three compare() methods belong to the Condition object. So I would rewrite that snippet (saving one line) as

$condition = \Drupal::database()->condition('AND')
  ->compare('d.entity_id', 'b.entity_id')
  ->compare('d.entity_type', 'b.entity_type')
  ->compare('d.field_name', 'b.field_name');

The CI tests passed, but just because the "Validatable config" test is allowed to fail.

TL;DR: I think it is worth looking at that test, but I think it is OK for it to fail for this issue.

I am not familiar with that test, but it seems to compare the results of

vendor/bin/drush config:inspect --statistics

on the target branch (11.x) and on the feature branch.

On the target branch, the installs drush and the config_inspector module. Then it installs the Standard profile and enables all core modules (except sdc, which is obsolete) and config_inspector. On the feature branch, the test first applies database updates.

The test fails if any of the statistics are lower on the feature branch than they are on the target branch.

I went through similar steps, but instead of vendor/bin/drush config:inspect --statistics, I exported configuration. The only difference between the two branches is that the feature branch includes core.base_field_override.node.article.promote.yml. It looks as though this config entity is not fully validatable: the schema is in core/config/schema/core.data_types.schema.yml, and it is not marked as FullyValidatable: ~. So it makes sense that the statistics go down and the test fails.

I am copying the participant list from the private meeting notes.

benjifisher → created an issue.

benjifisher → created an issue.

benjifisher → created an issue.

Add a brief description of Cross Site Request Forgery, with links to more complete explanations.

I agree it is a good idea to have an extra level of authentication before making changes to the codebase, but I think that "password authentication" is too restrictive.

Some sites use single sign-on (SSO) or other methods of authentication, so requiring a password would not be appropriate. On the other hand, it may be that sites using SSO also have dev, stage, and prod versions of the site, so they could use the proposed dev/prod toggle.

It is out of scope for this issue, but I think that Drupal core should provide an authentication API. That would make it possible, in this issue, to let the site owner choose the method of authentication. It would also simplify the tfa (Two Factor Authentication) module.

Usability review

We discussed this issue at 📌 Drupal Usability Meeting 2025-04-25 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, @shaal, and @worldlinemine.

We like the idea of breaking up the search page, as this issue does. But instead of making two separate pages (with two entries in the Administration menu) we would like to have two tabs (with one entry in the Administration menu). This is similar to the approach used for Languages (/admin/config/regional/language), where the first tab has a list of available languages and the second tab (Detection and selection) has settings.

Once the list of search pages is on a separate page (or tab) we also think it would be helpful to add some help text to that page. Again, the Languages page (List tab) is a good model: it explains where the ordering and default are used.

We also spent some time discussing the "Indexing progress" section, and whether it belongs on the main tab (list of search pages) or the settings tab. In the end, we agreed with the decision made here, to keep it with the settings.

We also think that a useful addition would be to have a button that immediately runs cron, just for the Search module. That button should go on the main tab, where the indexing progress of each page is listed. But it should not really be "immediate": it should open a confirmation form, just like the "Re-index site" button. The confirmation form should check whether cron is currently running; if so, then offer options to check again or to cancel. Otherwise, give some information (something like "314 of 1000 items are indexed; continue to add 100 items to the index"), a link to the settings page, and a confirmation button.

But that is out of scope for this issue.

In summary:

1. Recommendation: use two tabs instead of two separate pages.
2. Suggestion: add some help text to the main tab (list of search pages).
3. Follow-up issue: add an "Index now" button to the main tab.

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

benjifisher → created an issue.

Updated from discussion on the Migrate video call today. benjifisher, heddn, mikelutz and quietone were present. 📌 [meeting] Migrate Meeting 2025-04-24 2100Z Active

Two of the blocking issues have been Fixed, and the third is Closed (won't fix).

Updated from discussion on the Migrate video call today. benjifisher, heddn, mikelutz and quietone were present. 📌 [meeting] Migrate Meeting 2025-04-24 2100Z Active

Two of the blocking issues have been Fixed, and the third is Closed (won't fix).

Updated from discussion on the Migrate video call today. benjifisher, heddn, mikelutz and quietone were present. 📌 [meeting] Migrate Meeting 2025-04-24 2100Z Active

We discussed this issue at 📌 [meeting] Migrate Meeting 2025-04-24 2100Z Active and decided to close it as Won't Fix.

The minimum_version and requirements_met attributes are all or mainly used by the migrate_drupal module, but they are Mostly Harmless, so we decided not to spend any effort on removing them.

I am adding a rough transcript and adding related issues. See also the child issues of 📌 Tasks to deprecate Migrate Drupal Active .

Rewrite and expand the documentation. Include

• CSRF protection in the Form API
• The _csrf_confirm_form_route option
• The _csrf_request_header_token requirement
• The system.csrftoken route

Add "Structure of routes" as related content. It is a sibling of the parent guide.

Here are a few suggestions for the agenda:

1. Converting source plugins from annotations to attributes
2. Deprecating the migrate_drupal and migrate_drupal_ui modules
3. Editing migrations in the admin UI

We converted source plugins to attributes in 📌 Convert MigrateSource plugin discovery to attributes Active . In a follow-up issue, 📌 Move source_module and destination_module from Migrate to Migrate Drupal Needs work , we removed source_module from the attribute class and reverted all the source plugins that extend DrupalSqlBase to use annotations (since they need to declare source_module).

Are there any other follow-up issues that need attention? Maybe 📌 Remove mimimum_version and requirements_met from MigrateSource attribute class Active ? If we want to do that, should we push to get it in before Drupal 11.2.0 is released? (That will be the first release that includes the attribute class, so BC is much less problematic if we change the attribute class before then.)

Other recently fixed issues: we moved two source plugins from migrate_drupal to migrate in 📌 Move content_entity source plugin to migrate module Active and 📌 Move the d8_config source plugin to the migrate module Active . Now we can have same-site migrations, using those source plugins, without enabling the migrate_drupal module. I now feel much better about deprecating the migrate_drupal module. Should we go ahead and do it? Let's review the plan issue, 📌 Tasks to deprecate Migrate Drupal Active .

When I added the Import CSV Recipe → , I realized that we are missing one important step for "ambitious site builders": they have no good way to edit (or even inspect) migration plugins. I think the Modeler API → module (currently in development) will help fill that need. It pulls out the code from the ECA → module that interacts with the JS-based editor. When it is ready, it will provide a tool that we can use to build a great editing experience for migrations (defined as config entities thanks to the migrate_plus module). First step: get the modeler_api module to an alpha release. (I think it is close.)

benjifisher → created an issue.

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.