🇺🇸United States @benjifisher

At 📌 Drupal Usability Meeting 2025-02-21 Active , we discussed the text for Selection Lists: @aaronmchale, @benjifisher, @simohell, @worldlinemine.

mcdruid → credited benjifisher → .

mcdruid → credited benjifisher → .

mcdruid → credited benjifisher → .

benjifisher → created an issue.

benjifisher → created an issue.

benjifisher → credited benjifisher → .

@godotislate:

Thanks for the updates. I agree: you have addressed all my comments on the MR.

The only changes since my last round of testing (Comment #81; see also Comment #79) are a couple of comments and one test file, so I do not need to re-test.

Let's move this issue along!

I did some more testing today.

Locally, I merged with the current 11.x branch.

Testing with migrate_drupal

First, I installed a Drupal 7 site and a Drupal 11 site (using the feature branch). On the Drupal 7 site, I created 100 article and page nodes, using the devel_generate module. On the Drupal 11 site, I installed the migrate_drupal_ui module. I then imported everything from the Drupal 7 site.

There were just a couple of migration messages (from the user migration, which ran into some unsupported permissions, and the d7_system_file migration). I got the same messages when I repeated the test with the 11.x branch.

All the content (users, files, taxonomy terms, nodes) imported as expected.

Testing with migrate_plus

I re-installed Drupal 11 (using the feature branch). I then added the Import CSV Recipe → , which installs some contrib modules (migrate_plus, migrate_source_csv, migrate_source_ui) and a few migrate_plus.migration.* config entities. Following the instructions in the README, I created some users, taxonomy terms, and nodes using the example CSV files from the recipe. It all worked as expected.

Summary

I see no problems in today's manual testing.

Move to the bottom of the menu.

Since the page title does not have "a", the default sort order list this page ("XML, JSON, ...") before the similar pages ("a CSV source", "a SQL source", etc.).

Rest the menu weight.

Rest the menu weight.

Rest the menu weight.

Add <code> tag around a method name.

Move near the top of the menu.

I suggested a couple of different plugin IDs, but I think I prefer config_entity as a counterpart to the existing content_entity source plugin.

I have two recipes that leverage the Migrate API. The second one is pretty minimal for now (and it has only a dev release).

1. Import CSV Recipe →
2. Migrate Quickstart →

We continued the discussion at 📌 Drupal Usability Meeting 2025-02-14 Active : @benjifisher, @rkoller, @simohell, and @worldlinemine.

It turns out that Selection List is more complicated that I anticipated.

benjifisher → created an issue.

I think we should also re-implement the d8_config source plugin (see Comment #12 here). I added the child issue 📌 Move the d8_config source plugin to the migrate module Active .

More on the same point: the constructor for DrupalSqlBase sets the $entityTypeManager property, which is not used in the Config class.

The getSystemData() and variableGet() methods will not work with a D8+ source because they need the system and variable database tables. Most of the other methods in the class depend on getSystemData().

Also, DrupalSqlBase extends SqlBase in the migrate module. The base class defines getDatabase(), which is the basis for connecting to a secondary database. So the new class can extend SqlBase directly and still generate config items from the current database or another one.

There are some differences between this issue and 📌 Move content_entity source plugin to migrate module Active .

The plugin ID d8_config is outdated. As part of this issue, do we want the new plugin ID to be config or drupal_config? If so, do we still want to remove the plugin annotation (or attribute after 📌 Convert MigrateSource plugin discovery to attributes Active ) from the existing class?

This class extends DrupalSqlBase, and we probably do not want to keep that class. Should we migrate some parts of that class (properties, methods) into the new plugin class? Should we create traits?

My initial thought is that we want to leave as much as possible behind. That means that the new plugin is more of a re-implementation than a straight move, which also argues in favor of choosing a new plugin ID. For example, DrupalSqlBase implements Drupal\Component\Plugin\DependentPluginInterface and uses Drupal\Core\Entity\DependencyTrait:

  public function calculateDependencies() {
    // Generic handling for Drupal source plugin constants.
    if (isset($this->configuration['constants']['entity_type'])) {
      $this->addDependency('module', $this->entityTypeManager->getDefinition($this->configuration['constants']['entity_type'])->getProvider());
    }
    if (isset($this->configuration['constants']['module'])) {
      $this->addDependency('module', $this->configuration['constants']['module']);
    }
    return $this->dependencies;
  }

I think the new plugin does not need that.

benjifisher → created an issue.

The patch from #35 does not apply to the 6.0.x branch.

1. My version of git did not like the DOS-style line endings.
2. My version of git did not like that there were some empty lines.
3. There is a conflict from 🐛 Fix failing tests on HEAD Fixed .

I fixed (1) and (2) by editing the patch file. (I added a single space to the empty lines.)

I fixed (3) by checking out the commit before that issue, applying the patch, committing, and then rebasing on the 6.0.x branch.

It looks as though I did some of the early work on this issue, but I have not looked at the code changes in years. I am setting the status back to NR. Someone who has worked with this issue more recently should review and re-test after my rebase.

I created a MR for this issue, and I am attaching a patch for convenience.

benjifisher → created an issue.

Since we are not adding tests to the 10.3.x branch, the current MR does not have any of my code. So I think I am eligible to review this issue: RTBC.

@smustgrave: As you pointed out in Comment #4, the same change, applied to the 11.x branch, causes a test failure. Looking at that failing test, I think the route subscriber is auto-wired (and so it is considered a bug to add the tag the old way).

I just noticed this issue, and I had a look at the commit:

--- a/core/lib/Drupal/Core/Cache/CacheTagsChecksumTrait.php
+++ b/core/lib/Drupal/Core/Cache/CacheTagsChecksumTrait.php
@@ -160,6 +176,13 @@ public function reset() {
+  /**
+   * Implements \Drupal\Core\Cache\CacheTagsChecksumPreloadInterface::registerCacheTagsForPreload()
+   */
+  public function registerCacheTagsForPreload(array $cache_tags): void {
+    $this->preloadTags = array_merge($this->preloadTags, $cache_tags);
+  }

Should we add array_unique() to that?

Second point:

--- /dev/null
+++ b/core/lib/Drupal/Core/Cache/EventSubscriber/CacheTagPreloadSubscriber.php
@@ -0,0 +1,52 @@
...
+/**
+ * Preloads frequently used cache tags.
+ */
+class CacheTagPreloadSubscriber implements EventSubscriberInterface {
+
+  public function __construct(protected CacheTagsChecksumInterface $cacheTagsChecksum) {
+  }
+
+  /**
+   * Preloads frequently used cache tags.
+   *
+   * @param \Symfony\Component\HttpKernel\Event\RequestEvent $event
+   *   The request event.
+   */
+  public function onRequest(RequestEvent $event): void {
+    if ($event->isMainRequest() && $this->cacheTagsChecksum instanceof CacheTagsChecksumPreloadInterface) {
+      $default_preload_cache_tags = array_merge([
+        'route_match',
+        'access_policies',
+        'routes',
+        'router',
+        'entity_types',
+        'entity_field_info',
+        'entity_bundles',
+        'local_task',
+        'library_info',
+        'user_values',
+      ], Settings::get('cache_preload_tags', []));
+      $this->cacheTagsChecksum->registerCacheTagsForPreload($default_preload_cache_tags);
+    }
+  }

I think we should add something to core/assets/scaffold/files/default.settings.php describing the new setting.

In addition to the comment changes I requested yesterday, I think the new test needs a little change. Back to NW for that, but I think those are all easy changes to make.

I did some manual testing:

1. Switch to 11.x or the feature branch.
2. Install the standard profile.
3. Enable the migrate module.
4. Use drush php to exercise the plugin manager.
5. Enable the migrate_drupal module.
6. Repeat Step 4.

In Steps 4 and 6, I used the following commands. I am adding linebreaks here for readability, but I used one line to reduce the output.

$source_plugins = Drupal::service('plugin.manager.migrate.source')->getDefinitions();
foreach ($source_plugins as $key => $plugin) {
  ksort($plugin);
  $source_plugins[$key] = $plugin;
}
$source_plugins

I am attaching the results as two text files (one for 11.x, one for the feature branch).

I see the following differences:

1. In 11.x, the value of the 'provider' key is an array. In the feature branch, that array is assigned to 'providers', and the value of the 'provider' key is the first element of that array.
2. In the feature branch, every plugin definition includes "minimum_version" => null,.
3. In 11.x, every plugin derived from content_entity includes "deriver" => "\Drupal\migrate\Plugin\migrate\source\ContentEntityDeriver",. In the feature branch, the leading '\' is missing.
4. In 11.x, every plugin derived from content_entity includes "source_module" => "migrate",. In the feature branch, it is "source_module" => "migrate_drupal",.

Of these, (1) is intentional. (2) may not be important, but perhaps it is worth being consistent. Probably it is worth changing (3). And (4) is a problem: 📌 Move content_entity source plugin to migrate module Active moved the content_entity source plugin to the migrate module and updated the source_module annotation, but the current MR missed that. This issue definitely NW for that.

I have reviewed most of the changes in the MR. So far, I think all my suggestions are related to comments (including doc blocks).

There are about 115 files where the only changes are replacing annotations with attributes. Even if I ignore all the source plugins, that leaves 18 files, +468/-54 lines, which is more than enough for one MR. So I am convinced that dealing with source_module in a separate issue is the right decision.

I want to spend some more time reviewing the new automated tests, and I want to do some manual testing. Other than that, and the comment changes I suggested on the MR, I think this issue is ready to go. It is a good sign that the existing tests needed only minor updates.

Given the complexity of this MR and that it is close to ready, I think we should finish this issue instead of waiting on other issues that might make it simpler. We still have a nearly three months before 11.2.0-alpha1 is released. That should give us time to work on some of those issues after this one is fixed.

When you tag an issue for usability review, please make it easy for the usability team to review the issue. Update the issue summary:

• The "Proposed resolution" section should describe all the changes made in the issue.
• The "User interface changes" should show the existing UI and the proposed UI.
• The "Remaining tasks" should include one explaining the usability issue(s).

Most of the time, I prefer to have plain text in the "Proposed resolution" section and screenshots in the "User interface changes" section.

You can also attend the weekly usability meeting to present an issue.

I am adding the tag for an issue summary update and setting the status to NW.

@rkoller, @worldlinemine, and I continued the word-smithing at 📌 Drupal Usability Meeting 2025-02-07 Active .

@alexpott:

Maybe we can make query() protected as part of 📌 Improve separation of concerns in SqlBase Active .

The tests still pass.

benjifisher → created an issue.

One option, but not a very practical one, is to save SVG files as private files, not public. Then (somehow) Drupal could sanitize them on output.

I think a better option is to provide a file validator using a suitable PHP library. Then site owners and contrib module maintainers would have the option of adding SVG validation to any field that allows uploading SVG files. We could recommend using it and add it to the Standard and/or Umami profiles. But site owners would have the freedom to use it or not.

This option requires finding a suitable library that implements something like isSvgSafe(), returning bool. I have not researched such libraries.

One concern I have heard is that new vulnerabilities might be discovered in SVG files. The library we use might be updated, preventing new uploads of malicious files, but that would not help if such files have already been uploaded. I think a good way to deal with that would be to provide a field formatter that returns a boolean (Safe or Unsafe). We could add that formatter to /admin/content/files. After updates to the SVG library, site owners could filter on that value to search for existing, unsafe SVG files.

The meeting just finished, and I am adding issue credit for the attendees.

benjifisher → created an issue.

@quietone:

From Comment #11:

This issue only affects the 10.3.x branch.

So we may decide not to fix this issue, but moving it back to the 11.x branch does not make sense.

Something went wrong with the link in your comment, but the list at https://www.drupal.org/about/core/policies/core-change-policies/allowed-... → does not mention security fixes, which seems odd to me. The security team decided that this issue can be fixed in public, as a "security improvement" or "security hardening". I think that should make this issue eligible.

If new tests are not allowed, then that makes it easier. I removed the test and updated the MR.

I rebased and resolved the merge conflict.

The only conflict was that this issue added a missing @return comment, and a different (more generic) one was added in 📌 Fix Drupal.Commenting.FunctionComment.MissingReturnComment in non-tests Needs review . I kept the comment added in this issue.

Back to RTBC.

Are there any nodes in Umami that customize their layout? I think not:

MariaDB [db]> SELECT * FROM node__layout_builder__layout;
Empty set (0.001 sec)

I think that means that, in its current state, Umami does not need #3160146 in order to have its content saved in the default_content format used by recipes.

I was hoping that Umami would configure its display modes to "Use Layout Builder" but not "Allow each content item to have its layout customized." Unfortunately, the Full Content display mode of each content type has both options selected.

This issue is at least related to 🐛 Warnings logged for missing field block plugins when enabling layout builder on an entity view display for the first time Active , perhaps a duplicate.

I am adding the STR from Comment #3 to the issue summary. I have noticed the same thing.

Now that 🐛 Move content_translation I18nQueryTrait to migrate module Active is Fixed, we can change this one from PP-2 to PP-1.

I tried adding \Drupal::service('router.route_provider')->reset(); to the test, but that did not help.

Now that 🐛 Move content_translation I18nQueryTrait to migrate module Active is Fixed, we can un-postpone this one. The MR needs to be updated (simplified) after the work in that issue.

This issue still needs an issue summary update, now that we have agreed on the approach. (Again, that means simplifying.) This issue already has the tag for that.

I published the change record, and I am about to un-postpone 📌 Convert MigrateSource plugin discovery to attributes Active .

The tests all pass now that the original commit is made on the 10.3.x branch.

I tried adding a test, but it is not working. :-( At least, it fails when I run it locally. I added the test to the MR anyway: maybe someone else can see what is wrong.

I am adding 📌 Conditionally disable access to update manager routes Fixed as a related issue, since that is the issue that introduced the bug.

This issue only affects the 10.3.x branch. I am updating this issue and changing the target of the MR. Let's see if that resolves the failing test.

This bug report needs tests. I am adding the tag for that and leaving the issue at NW, even if the tests pass.

I am adding issue credit for those who commented on the private issue.

Somehow I missed that 🐛 Update manager routes are not disabled anymore when allow_authorize_operations is FALSE Active was created a few days ago. I am closing this issue as a duplicate.

I implemented the proposed resolution, and I started working on a test. My test does not work, and I am not sure why. I committed it to the feature branch anyway. Maybe someone else will have a chance to look at it.

benjifisher → created an issue.

We now have a version that addresses the request in #55 and also replaces the Functional test with a Kernel test. Back to RTBC.

@godotislate:

Good find! Yes, a kernel test is much better.

On the principle that no good deed goes unpunished, I made several suggestions for changes on the MR.

We do not need both MRs. Either cherry-pick the last two commits from MR 10246 to MR 11081 or else cherry-pick the last commit in the other direction. (Or make one commit on MR 10246, incorporating my feedback.)

Maybe use a data provider so that the test-only job fails on all 5 source plugins instead of getting a fatal error on just the first one.

Finally, I think we may want to reuse the first part of the test (the part that breaks discovery for uninstalled modules). Eventually, we should move it to a trait, but I think it is premature to do that now, so let's start by making it a separate method in the test class. It should have at least one argument: a list (array) of module names to update.

I am not sure: for this test, is it better to break class discovery for all uninstalled modules or just for content_moderation? I am leaning toward the second option. That would also save a few lines of code.

If you prefer the first option, then the new method should default to acting on all uninstalled modules. And it should have a required first argument of module that can be asserted to be on the list: something like disablePsr4(array $expected, ?array $disabled = NULL): void. Maybe use longer, more descriptive, parameter names.

The last few commits address the suggestion in Comment #55. Back to RTBC.

benjifisher → created an issue.

I reviewed the changes. There is just one problem: we should add parameter type declarations as well as a return-type declaration. Back to NW for that.

On the plus side, I see that the earlier version of the MR only did part of the recommended deprecation. (It added @trigger_error() below the namespace line, but it did not add @deprecated and @see link-to-cr to the doc block). The current version fixes that: thanks.

Reference: https://www.drupal.org/about/core/policies/core-change-policies/how-to-d... →

I also updated the change record (CR). It still targeted 10.3.x, and I updated that to 11.2.x. It said that several classes that use the trait were also being moved to migrate_drupal, which was the plan before Comment #31. I removed that section of the CR.

There were merge conflicts in the PHPStan baseline file.

I rebased on the 11.x branch, skipping the commits that affected the baseline file. I also used the opportunity to simplify the git history. Then I manually removed the lines from the baseline for the removed test class and added an ignored error for the new version of that file with

phpstan analyze -c core/phpstan.neon.dist -b core/.phpstan-baseline.php

Back to NR to make sure I did not mess it up with my rebase. It looks as though PHPStan has already approved of the changes.

Yes, the issue should go back to NR. I will review it soon, unless someone else does it first.

I think we made the same decision on 📌 Move content_entity source plugin to migrate module Active : keep the existing class intact and add type declarations to the new one. That issue seems to have merge conflicts, and I think I have time to handle that now.

@joachim, @dcam:

Good news! We plan to keep the content_entity source plugin. In fact, 📌 Move content_entity source plugin to migrate module Active is currently RTBC.

It looks as though I created the issue fork, but forgot to create the MR, when I made Comment #5. I just did that, and rebased the feature branch on the latest 2.0.x.

benjifisher → created an issue.

Add https://www.drupal.org/docs/develop/issues/issue-procedures-and-etiquett... → as related content.

@heddn:

CR added and feedback on MR addressed.

Thanks for that. I made a few edits to the change record, and I reviewed the changes. LGTM

Then I remembered the T in RTBC.

I decided that I should do something close to a real-world test. I installed Drupal 7 and created 50 nodes with the devel_generate module. I checked the D7 database: there are 27 Article nodes, 9 of which have nid less than 17.

I set up a D11 site to connect to the D7 site: in settings.php,

$settings['migrate_node_migrate_type_classic'] = FALSE;
$databases['migrate']['default'] = [
  'database' => 'db',
  'username' => 'db',
  'password' => 'db',
  'host' => 'ddev-drupal7-db',
  'driver' => 'mysql',
];

I enabled the migrate_drupal and mymigration modules, including this implementation of hook_query_TAG_alter():

function mymigration_query_migrate_alter(AlterableInterface $query) {
  if ($query->hasTag('migrate_d7_node:article')) {
    $query->condition('n.nid', 17, '<');
  }
}

With 11.x, I see all 27 Article nodes:

benji@drupal-web:/var/www/html$ drush cr && drush ms d7_node:article
 [success] Cache rebuild complete.
 ----------------- -------- ------- ---------- ------------- --------------- 
  Migration ID      Status   Total   Imported   Unprocessed   Last Imported  
 ----------------- -------- ------- ---------- ------------- --------------- 
  d7_node:article   Idle     27      0          27                           
 ----------------- -------- ------- ---------- ------------- ---------------

With the feature branch, I see just 9:

benji@drupal-web:/var/www/html$ drush cr && drush ms d7_node:article
 [warning] include_once(/var/www/html/core/modules/datetime/datetime.module): Failed to open stream: No such file or directory Extension.php:160
 [warning] include_once(): Failed opening '/var/www/html/core/modules/datetime/datetime.module' for inclusion (include_path='/var/www/html/vendor/pear/archive_tar:/var/www/html/vendor/pear/console_getopt:/var/www/html/vendor/pear/pear-core-minimal/src:/var/www/html/vendor/pear/pear_exception:.:/usr/share/php') Extension.php:160
 [success] Cache rebuild complete.
 ----------------- -------- ------- ---------- ------------- --------------- 
  Migration ID      Status   Total   Imported   Unprocessed   Last Imported  
 ----------------- -------- ------- ---------- ------------- --------------- 
  d7_node:article   Idle     9       0          9                            
 ----------------- -------- ------- ---------- ------------- --------------- 

The warning goes away if I clear caches a second time. It is probably because the feature branch does not have the commit for 📌 Deprecate views_field_default_views_data Active .

I can now honestly say this issue is RTBC.

Remove duplication: "merge request or merge request". Perhaps it originally read "merge request or patch" and someone did an automated search-and-replace, changing "patch" to "merge request".

hestenet → credited benjifisher → .

hestenet → credited benjifisher → .

If I understand correctly, with-chrome means to use the service (Docker container) $_CONFIG_DOCKERHUB_ROOT/chromedriver:production, and with-selenium-chrome: means to use the service selenium/standalone-chrome:127.0. (Presumably the version will be updated as needed.)

After the change in MR 11021, the only tests in pipeline.yml using with-chrome will be 🖱️️️ PHPUnit Functional Javascript (non W3C legacy) and 🚲 Performance tests.

I notice there is also a 🩹 Test-only changes job defined in pipeline-test-only.yml. That job already has the service selenium/standalone-chrome:latest.

I am not at all sure how all this fits together, but

1. The change in MR 11021 (switching to selenium-standalone-chrome looks like it will make the two jobs more consistent.
2. I am surprised that there are two jobs with so much in common and yet not identical.

Fix broken links in the list of former members.

Add former members who were missing from the list. We compared users on s.d.o with the "former team members" role to the list here. The one account we skipped is "Issue administrator".

Add accents to chx's name. This is the version chx provided in a Slack thread.

There have been 4 team leads to date, not 3. mlhess is the current one. According to https://www.drupal.org/about/core/policies/roles-and-responsibilities/pa... → , the first three were chx, heine, and greggles.

Add chx as a former member. In fact, chx is a former team lead according to https://www.drupal.org/about/core/policies/roles-and-responsibilities/pa... → .

We continued the word-smithing at 📌 Drupal Usability Meeting 2025-01-24 Active : @benjifisher, @rkoller, @simohell, and @worldlinemine.

benjifisher → created an issue.

benjifisher → created an issue.

I feel the same way. Thanks for all your contributions!

@kul.pratap:

Thanks for making the changes I suggested!

I see that you have several contribution credits already, but none yet for Drupal core. Maybe this will be your first. Good luck, and I hope you keep contributing!

I reviewed the changes in the MR, and they do what I requested in the issue summary. These changes are all on comment lines (the class doc block) so there is no need for testing.

Once these changes are merged into the 11.x branch, we should see them in the API docs: https://api.drupal.org/api/drupal/core%21modules%21migrate%21src%21Plugi....

quietone → credited benjifisher → .

benjifisher → created an issue.

I added 📌 Improve separation of concerns in SqlBase Active , so I am removing the tag for a followup issue.