🇺🇸United States @benjifisher

Possibly related: 🐛 Running the installer leads into a wsod Active .

I am linking to some issues that seem to be related.

Maybe this should be a separate issue, but today I noticed that I cannot set a menu item to Expanded if it is created by Views.

We discussed this issue at ✨ Warn user when entity delete will cause menu item re-parenting Needs review . That issue will have a link to a recording of the meeting.

Thanks for working on this issue. At least one attendee has run into exactly this problem, and there were a lot of child menu items involved.

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

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.

Usability review

We have a few recommendations:

1. Shorten the text in the confirmation form.
2. Suggest changing the parent of the menu item before deleting it.
3. Special treatment when deleting custom menu links.
4. Follow-up issue: provide an option to delete the child menu items.

Shorten the text

There is room for improvement, but this is the best we could do during the meeting. After the standard "This action cannot be undone.", either

Deleting this [content item] will make these [count] child menu items top level:
...

or

Deleting this [content item] will move these [count] child menu items under [parent link]:
...

Make adjustments for singular or plural, as the current MR already does.

Suggest changing the parent

A good status message starts by describing the situation, but then it suggests what to do about it. In this case, a common action will be to change the parent of the menu item that is going to be deleted: then, after deletion, all the child menu items will be children of the new parent.

Changing the parent item can be done from the menu page (where all the items in the menu are listed, with a click-and-drag interface), from the edit page for the individual menu link (where the parent can be selected from a list) or from the edit page of the content item. We feel that the click-and-drag interface is problematic. There might be permission issues with editing the menu item, but that gives the most flexibility.

If the user is deleting a content entity (not a custom menu link ... see the next point) and the suggestion is to edit the related menu link, then it would help to give a link to that edit page.

We do not have suggested text, but this information can go after the list of affected child items.

Special treatment when deleting custom menu links

Custom menu links are content items, so the warning gets added to the confirmation form when deleting a custom menu link. So far, so good! But the text on the current MR, when deleting a custom menu link, includes "The menu items for this custom menu link", which does not make sense. Pay attention to the case where the content item being deleted is a custom menu link.

With the text proposed earlier, that entire sentence is removed, so maybe we do not have to worry about it. But keep this case in mind while continuing to work on this issue.

Follow-up issue

Sometimes, the user will want to delete the child menu items instead of move them to a different parent. There are a lot of things to be considered here, so we recommend adding a follow-up issue to discuss how to implement this. Maybe the right thing to do is turn the menu page into a form that allows bulk operations. Maybe there should be an option on the confirmation form, or the edit form for a custom menu link, to delete children. Should that include all descendants? How can we make it clear whether we are deleting content entities or their related custom menu links?

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.

I am worried about how disruptive this change could be. Since both @catch and @daffie have commented on this issue, and neither seems concerned about that, I guess it is OK. But I am adding the tag for a change record (NW for that) and I am adding a release-notes snippet to the issue summary.

benjifisher → created an issue.

benjifisher → created an issue.

I commented on the three linked issues. (I made one long comment and two short ones.)

Usability review

We discussed this issue briefly at 📌 Drupal Usability Meeting 2024-12-13 Active . That issue has a link to a recording of the meeting.

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

We agree with the recommendation of this issue. The Search and Filter controls affect what is shown, and that section of the form is already pretty complicated. The Sort control should be moved out of that section and next to the Display toggle (list or grid).

We did not discuss Items per page, but I think that also belongs above the results, not next to the pager.

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

Usability review

We discussed this issue briefly at 📌 Drupal Usability Meeting 2024-12-13 Active . That issue has a link to a recording of the meeting.

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

We agree with the recommendation of this issue: having separate tabs for the different sources (especially core modules vs. contrib modules) is not a good user experience. Users want a module to provide a feature for their site, and they do not care whether the module comes from core or contrib. (At least, that is not their first concern.)

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

Usability review

We discussed this issue at 📌 Drupal Usability Meeting 2024-12-13 Active . That issue has a link to a recording of the meeting.

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

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).

For this issue, the issue summary described a problem that has already been fixed. After we tested that, we had to read the rest of the comments to figure out what the open questions are.

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

At the meeting, we discussed the various controls: filters, sort order, and display (list or grid). For each of these, there are reasons to have different choices depending on the source:

• Filters: The categories for modules are different from the categories (yet to be determined!) for recipes. When themes are added to the project browser, they are likely to have yet another set of categories. The statuses of security, maintenance, and development apply to contrib modules but not to core modules (except for development status of experimental modules).
• Sort order: it does not make sense to sort core modules by recency, but this is an option for contrib modules. Relevance comes from some external data (Solr or ElasticSearch, I think) and is not available for core modules.
• Display: Currently there is very little information to display on recipes, compared to modules, so users might prefer different displays. When and if we add themes, users will want to choose a different display for them.

For each control, considered by itself, that suggests having a setting for the source (tab) that is independent of the other sources (tabs). If I choose a filter, sort order, or display for contrib modules, then I want those choices after switching to another tab and then switching back to contrib modules.

Considering the various controls together makes the decision clearer. We do not want the choice of filter to be tied to the source (tab) but the choice of display to affect all sources (tabs).

If we did want one of these choices to affect all sources, then the control should be moved out of the tab, but let's not do that.

Some related observations:

• Do not show result counts for the other sources. Keep the count above the results, and remove it from the tab.
• We know that there are technical reasons for keeping core modules separate from contrib modules, but users do not care about that distinction. They would rather see all modules in the same place. It may lead to confusion as modules are moved from core to contrib.

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

In MR !338, I added the empty($install_state) check as suggested in #22, although I am not sure why it should be in drupal_cms_installer_library_info_alter() instead of drupal_cms_installer_theme_registry_alter().

If I were to going to try to fix this with only partial diagnosis, then I would clear caches after uninstalling:

function drupal_cms_installer_uninstall_myself(): void {
  \Drupal::service(ModuleInstallerInterface::class)->uninstall([
    'drupal_cms_installer',
  ]);
  // Clear caches here.
  // ...
}

That is based on my testing in #13: drush st shows Install profile : drupal_cms_installer, but that goes away after drush cr.

My guess is that @rkoller's stack trace in the issue summary (also reported by @rfay in #15 and @rkoller in #17) shows an NotFoundHttpException because it is trying to load a page defined by the dashboard module, and that module is (thanks to stale cache) apparently uninstalled.

If that guess is right, then you are looking at a symptom. If my guess is wrong, then you are probably looking in the right place.

larowlan → credited benjifisher → .

It is not related to this issue, but there is another problem with the launch-drupal-cms.sh script:

ddev config --project-type=drupal11 --docroot="$DOCROOT" || exit 3

The type drupal11 is only available with the latest DDEV. On Linux, I have installed with apt, and I seem to be stuck at 1.23.1. Perhaps that line should change to this (suggested by @rkoller):

ddev config --project-type=drupal --php-version=8.3 --docroot="$DOCROOT" || exit 3

After upgrading my Mac to Sonoma 14.7.2/Safari 18.2 (same as @rkoller) I can reproduce the problem.

I did not try exactly these steps to reproduce before upgrading. But I cannot use the script because I get an rsync error when I try ddev composer create .... Instead of fixing that, I use these steps:

mkdir cms-test14
cd cms-test14
ddev config --project-type=drupal11 --docroot=web
ddev start
ddev ssh
composer create-project drupal/cms --stability="RC" # on the container
vi .ddev/config.yaml # Change web to cms/web
exit
ddev restart
ddev launch # Set default browser to Safari.

I run the installer, without selecting any of the optional recipes. I end on the page https://cms-test14.ddev.site/admin/dashboard/welcome?check_logged_in=1 with the error message

Error: Call to a member function getPath() on null in drupal_cms_installer_theme_registry_alter() (line 280 of profiles/drupal_cms_installer/drupal_cms_installer.profile).

It seems that the custom installation profile did not uninstall itself:

% ddev exec cms/vendor/bin/drush st --field=install-profile
drupal_cms_installer

But if I clear caches, then it is gone:

% ddev exec cms/vendor/bin/drush cr                        
 [success] Cache rebuild complete.
% ddev exec cms/vendor/bin/drush st --field=install-profile && echo done

done

In fact, after clearing caches I do not see any problem with the site.

Using Firefox or Chrome, everything works normally.

My system:

• macOS Sonoma 14.7.1
• Safari 18.1.1
• Apple M3 Pro Chip
• 36 GB RAM
• DDEV 1.24.1 (*)
• lima 1.0.2 (*)

(*) After brew upgrade

I cannot reproduce the problem.

@rkoller:

You mention Safari, so I assume you are using macOS. What hardware, and what version of the OS?

What version of DDev are you using? And lima.

In #6, @phenaproxima mentioned possibly updating the testing instructions. Are these the steps you use?

• Check out the MR branch and copy the project_template directory to another location.
• Rename the copied directory to something else (like cms-test or whatever) and cd into it
• Edit launch-drupal-cms.sh and replace this line:
  ddev composer create drupal/cms --stability="RC"
  with this:
  ddev composer create drupal/cms --stability="dev" --repository='{"type":"vcs","url":"https://github.com/phenaproxima/test-ddev-cms.git"}'
• Run ./launch-drupal-cms.sh OR follow the proposed instructions from https://github.com/ddev/ddev/pull/6829

benjifisher → created an issue.

I checked the issue credits against the list in our private notes. LGTM

longwave → credited benjifisher → .

benjifisher → created an issue.

mr.baileys → credited benjifisher → .

xjm → credited benjifisher → .

benjifisher → created an issue.

I think you missed core/modules/menu_link_content/src/Plugin/migrate/source/d7/MenuLinkTranslation.php, so NW for that.

Can we really make the existing trait a wrapper for the new trait in the migrate_drupal module and not add any deprecation notice? Maybe we can, but it feels too easy. I guess the idea is that when we deprecate the new trait, that will automatically deprecate the existing one (the wrapper), and we have to do that before the next major version (D12).

The existing steps to reproduce (STR) involve a contrib module. That has two problems:

1. It is not clear from the STR that this is a bug in Drupal, rather than the contrib module.
2. Manual testing is complicated, since the fix is on a branch based on 11.x and the contrib module does not have a release compatible with Drupal 11.

I know, I can add the issue fork for the D11 compatibility issue as a package repository and install the contrib module that way, or I could use the lenient composer plugin. I said it is complicated, not impossible. ;)

We can solve both problems if we give different STR. Using drush php,

> \Drupal::service('plugin.manager.migration')->createInstance('d7_custom_block_translation')->getSourcePlugin();
PHP Fatal error:  Trait "Drupal\content_translation\Plugin\migrate\source\I18nQueryTrait" not found in /var/www/html/core/modules/block_content/src/Plugin/migrate/source/d7/BlockCustomTranslation.php on line 22

Fatal error: Trait "Drupal\content_translation\Plugin\migrate\source\I18nQueryTrait" not found in /var/www/html/core/modules/block_content/src/Plugin/migrate/source/d7/BlockCustomTranslation.php on line 22

Before running that code, I installed Drupal with the standard profile, enabled migrate_drupal and a custom module with a simplified version of d7_custom_block_translation:

id: d7_custom_block_translation
label: Content block translations
source:
  plugin: d7_block_custom_translation
process: {}
destination:
  plugin: null

The original version of d7_custom_block_translation is in the content_translation module.

Can we turn these STR into an automated test? I think so. I am adding the tag for tests.

We should update the issue summary with the improved STR. I can do that, but not now. I need some sleep. For now, another tag.

When I check out the feature branch from the MR, it fixes my manual test:

> \Drupal::service('plugin.manager.migration')->createInstance('d7_custom_block_translation')->getSourcePlugin();
= Drupal\block_content\Plugin\migrate\source\d7\BlockCustomTranslation {#7934}

It would be nice to get some manual testing of the original report: does the current MR fix the problem with the contrib entity_import module? Or does that require moving a lot more into the migrate_drupal module?

Questions from the issue summary:

The source_module property in source Annotations is specific to migrate_drupal. Should the source_module property be excluded from the new Attribute, especially since migrate_drupal is slated to be removed? See #53 📌 Convert MigrateSource plugin discovery to attributes Active and 📌 Move source_module and destination_module from Migrate to Migrate Drupal Needs work

That seems like a good idea, but I would like to decouple it from the other problems we have to handle in this issue. If we can get this issue done, then I think we can have a separate issue to remove source_module from Drupal\migrate\Attribute\MigrateSource, create a migrate_drupal class that extends it, and add source_module there.

... When Reflection is instantiated on classes that use unknown Traits, ... PHP will fatal error → . There are four plugin classes in the menu_link_content, block_content, and taxonomy modules that use the I18nQueryTrait in the content_translation module. To convert these plugins to use attributes, either ... or these 4 plugins and the trait need to be moved to live in the same module (see 🐛 Move content_translation I18nQueryTrait to migrate module Active ).

Why do we have to move the four source plugins? Isn't it enough to move the trait to migrate_drupal?

Alternatively, should these plugins (and/or all D6/D7 source plugins) remain unconverted and just keep using annotations until migrate_drupal is removed?

Are we going to start issuing deprecation notices when plugins use annotations instead of attributes? If so, then leaving a bunch of core plugins un-converted will break a lot of automated testing.

... does the automated/multiple provider discovery need to be recreated for Attributes?

We have to continue to support contrib and custom plugins, one way or another. I am not sure whether the automated discovery should be in Core, migrate, or migrate_drupal. In order to control scope, it might make sense to keep it in the migrate module for this issue, and then consider it along with the source_module attribute.

I am adding two more related issues mentioned in the issue summary.

I reviewed all the changes, and I do not see anything else that got lost.

I still think it is a bad idea to cast to string in EntityContentBase::fields(), and some of my other suggestions would be improvements. But I will not insist on those changes, so RTBC.

I skipped the initial comments and started reading from #32.

Thanks to @andypost (#38) for adding 🐛 Entity reference field View output is not used for selected entity display Needs work as a related issue. Using an Entity Reference view, we should be able to control both the available options and how the selected option appears.

In #45-#48, @philiph91 points out that an editor can change the text provided by the autocomplete widget, and whatever value is in parentheses is used as the entity ID. For example, using the Umami profile, the autocomplete might provide "Vegetarian (28)". If you change that to "Vegetarian (2)" and save, then you get "Baked (2)". I think this is a separate, but related, issue. See also #64. Some may consider this a feature, not a bug.

I see that #59 already reported the problem with the summary text that I mentioned in #82.

Comment #75 refers to "Custom References". Is that a contrib module?

Comments #76, #77 describe a bug when Taxonomy terms have commas in their names, but I cannot reproduce it. Perhaps it would help to have more explicit steps to reproduce.

The first line of the issue summary ends with 11.0.0. I am updating it to 11.1.0, to match the title.

I did a quick test with the feature branch, and I notice that the widget summary always shows Show entity IDs: Yes, whether or not I select the new option "Show Entity ID".

I rebased the feature branch on the current 11.x, pushed that to the issue fork, and created the new MR 10308. I am closing the old MR.

I closed ✨ Use data-* to check modules dependencies before submit Active , which we discussed at the 2024-11-08 meeting.

It has been two weeks since my previous comment, so I am going ahead with the plan to close this issue (works as designed).

I left a comment on 🐛 UX: Hide entity ID in autocomplete widget Needs work .

Usability review

We discussed this issue at 📌 Drupal Usability Meeting 2024-11-15 Active . That issue has a link to a recording of the meeting. For the record, the attendees at the usability meeting were @anmolgoyal74, @benjifisher, @rkoller, and @simohell.

It was difficult to understand the current state of this issue because the issue summary still suggests several possible resolutions even though it seems that there is consensus on what to do. I am adding the tag for an issue summary update. Also, the current MR uses the 11.x branch on the issue fork. That is the usual practice on many other projects, but Drupal keeps a copy of each standard branch (such as 11.x) in the issue fork, and we use a descriptive name for the feature branch. The current setup breaks our usual workflows.

We agree with the approach suggested in some of the comments. Since Drupal core already provides entity reference views for complex situations, all we need is an option to hide the entity ID in the widget. The decision to hide or not depends on the specifics of the site, so it should be left up to the site builder.

I see some discussion about implementing the new option in a way that it can be used or overridden by modules. That seems like a good idea, but the usability team does not have an opinion on whether to do that as part of this issue or as a follow-up.

There is also some discussion of whether to override the new option when there are duplicates in the list. We do not have a strong recommendation here, but there are a couple of reasons not to do that. It adds complexity (to the code and to the behavior of the new option). It overrides the intention that the site builder expressed by selecting the option. It is still confusing and not helpful for distinguishing the options (the basic reasons for this issue).

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

I am copy-editing the latest addition.

benjifisher → created an issue.

benjifisher → created an issue.

benjifisher → created an issue.

Fix up formatting.

Avoid contractions ("Here's"), shorten the text (Less is more!), and avoid a silly decision (is it "a SQL" or "an SQL"?).

heine → credited benjifisher → .

xjm → credited benjifisher → .

xjm → credited benjifisher → .

poker10 → credited benjifisher → .

poker10 → credited benjifisher → .

We accidentally made two issues for this meeting. I am closing this one as a duplicate of 🌱 Core security triage 2024-10-31 Active .

> Migrate existing security issues. Do we need closed issues, or lose those forever?

I think we need the closed issues. As a general rule, I hate to throw away information.

If an issue was considered a valid security issue, then fixed, then some of the important information is in the public SA. Even then, a lot of details about the possible exploit are only in the issue (now closed).

If an issue was considered not to be a valid security issue, then at best we have a comment in a public "hardening" issue. The discussion and the details are only in the issue (now closed). We often look for old issues similar to newly created ones: "in previous issues very similar to this one, we decided to fix them in public because ...".

I resolved several threads. I left open the ones where I made a suggestion and it has not been implemented. I still think these are improvements.

Unfortunately, one fix seems to have been lost, so back to NW. It is pretty small: just a missing |null in an @var comment.

johnpicozzi → credited benjifisher → .

johnpicozzi → credited benjifisher → .

Remember to check #1473760: Use data-* to check modules dependencies before submit at the 2024-11-22 meeting and close it if there are no further comments.

I checked the issue credits against the private notes that the security team keeps. LGTM

benjifisher → created an issue.

benjifisher → created an issue.

amateescu → credited benjifisher → .

I see two test failures on the latest pipeline (yesterday) for https://git.drupalcode.org/project/drupal/-/merge_requests/6111/pipeline... (for 📌 Implement Entity::fields() for migration destinations Needs work ):

1. Drupal\Tests\image\Functional\ImageStylesPathAndUrlTest::testImageStyleUrlAndPathPrivate
2. Drupal\Tests\responsive_image\FunctionalJavascript\ResponsiveImageFieldUiTest::testResponsiveImageFormatterUi

These two tests seem unrelated to the issue, and both tests passed when I re-ran them.

I forgot to review the failing tests:

1. Drupal\Tests\image\Functional\ImageStylesPathAndUrlTest::testImageStyleUrlAndPathPrivate
2. Drupal\Tests\responsive_image\FunctionalJavascript\ResponsiveImageFieldUiTest::testResponsiveImageFormatterUi

I added a note to 🌱 [meta] Known intermittent, random, and environment-specific test failures Active and re-ran the tests. They both passed.

@baltowen:

Thanks for fixing that.

Next time around, please add additional commits instead of making your changes as part of a rebase. I had to compare to the tag previous/2630732-11-x-migration-destinations-entity-fields/2024-11-05 in order to make sure that there were no accidental changes added during the rebase.

The merge conflicts were in core/modules/migrate/tests/src/Unit/destination/EntityRevisionTest.php. I reviewed that carefully, and the changes there look good to me. Back to RTBC.

I am replacing the screenshot in the issue summary and rearranging a bit to match the usual template. (Screenshots go under "User interface changes".) I am replacing "more appropriate text" with "empty text" in the Proposed resolution section.

My screenshot looks a lot like the one in 🐛 User logout is vulnerable to CSRF Fixed .

Previous comments and the Remaining tasks suggest adding an automated test, but I do not think that is needed.

Code looks good, the confirm form is what I show in the screenshot, and the confirm form works. RTBC.

Well, I tried to re-run the pipeline. I am not sure it is working.

If we do want automated tests, then we could test that the default text, "This action cannot be undone", does not appear on the page.

It is usually a bad idea to suppress phpcs warnings, but I think it is the right thing to do in this case.

There is a failure in Drupal\Tests\layout_builder\Functional\LayoutBuilderBlocksTest::testBlockPlaceholder. That test is already listed on 🌱 [meta] Known intermittent, random, and environment-specific test failures Active , so I am re-running the tests. (I do not see a way to re-run a single test suite, so I am re-running the entire pipeline.)

I have not tested, but the code change looks correct. I am setting the status to NR for testing and further review.

Comment #9 asked for automated tests, but I am not sure that is needed.

The issue summary needs an update (proposed resolution and screenshots), so I am adding the tag for that.

@baltowen:

Thanks for fixing that. There is still one line that needs to be fixed: see my comment on the MR. I wish I had caught that sooner.

There is also one unrelated test failure: Drupal\Tests\block\Functional\BlockCacheTest::testCachePerPage. Again, let's not worry about that until we fix the problem we know about.

@baltowen:

Thanks for working on this issue. Unfortunately, core/modules/migrate/tests/src/Unit/destination/EntityRevisionTest.php fails after the changes you made. Back to NW.

There are also some unrelated tests that fail. For now, do not worry about those.

This issue adds the $entityTypeBundle class variable to the test class. After your rebase, that variable is handled correctly: call reveal() in the setUp() method, not in the test methods. But your rebase also changes the way $storage is handled. That is not in scope for this issue, and Comment #4 on 📌 Drupal\Tests\migrate\Unit\destination\EntityRevisionTest uses weird mocking pattern Active explains why that one variable should not follow that pattern.

If you undo the changes to $storage in the test class, then that test should pass. Then we can worry about any unrelated test failures.