🇺🇸United States @benjifisher

There is a merge conflict because 📌 Convert test annotations to attributes in core's Unit tests Active changed the @dataProvider annotation to an attribute. I resolved the conflict by removing both the test and the data provider, as in the earlier version of the MR.

The merge conflict is because this issue added a function to NodeHooks1.php, which was removed in 📌 Remove NodeHooks1 Active . Following the recommendation in 📌 [meta] Clean up hook classes in core Active (and the text file attached to that issue) I moved the implementation of field_ui_preconfigured_options_alter() to a new class, NodeFieldHooks.

I also moved the implementations of the same hook in the taxonomy and user modules to new classes (TaxonomyFieldHooks and UserFieldHooks). When the meta issue reaches those two modules, a little bit of the work will already be done.

Another fix would be to move the hook above the opening of the group:

/**
 * @addtogroup field_purge
 * @{
 */

In fact, it would be a good idea to reorganize all the hooks in field.api.php, but that would not be a Novice task. In the Proposed resolution, I recommend moving the hook after the close of the group for two reasons:

1. It is the minimal change to fix the problem.
2. If another issue adds a new hook to the end of the file, then that new hook will be outside the group.

benjifisher → created an issue.

My understanding is that config schemas are for config entity types. In theory, we could have a config schema for migrate_plus.migration.*, and in theory we could generate a config form from that schema. In practice, I do not see how we can get that to work unless we have some way for process plugins to specify their config forms.

Background: the main part of any migration (core plugin or migrate_plus config entity) is the process section. This consists of several process pipelines. Many pipelines are single steps (like assign a source value to a destination value) but some pipelines are long chains of process plugins, each with its own configuration.

Source, process, and destination plugins are not config entities, so config schemas do not apply directly. In Comment #3, I suggested an approach to getting config schemas to apply. But that is still in the ideation phase, a few steps before vaporware.

benjifisher → created an issue.

I am copying the list of participants from the Security Team's private notes.

benjifisher → created an issue.

@thejimbirch: Thanks for opening this issue.

@jurgenhaas: There are only a few source and destination plugins, but there are a lot of process plugins. These are PHP classes, generally extending Drupal\migrate\ProcessPluginBase. So I am not sure what config schema you mean.

When I called it "a long-term project", I was thinking of the old issue 📌 Create a trait and base class to implement \Drupal\Component\Plugin\ConfigurableInterface Needs work , which was Fixed 2 or 3 months ago (2025-06-14). I think the next step is to expand Drupal\Component\Plugin\ConfigurableInterface, or add a related interface, with some sort of form-builder method. Then configurable plugins would have a consistent way to provide their configuration forms.

What that new, public method would be, and how it is implemented, is up for discussion. Maybe it would involve a config schema, at least as one option. Or maybe it would involve a config entity type, and the config schema would declare that it handles that type.

That is why I describe this issue as providing a proof of concept. For now, then suggested module is responsible for providing config forms for a limited number of plugins.

The suggested module would not be a candidate for Drupal core. Since it relies on migrations defined by config entities, it has to depend on the migrate_plus module. I suggested using the csv source plugin for the proof of concept (and for making something that is actually useful). That comes from another contrib module, migrate_source_csv.

benjifisher → created an issue.

There was no meeting on August 14, so I am repurposing this issue for the August 28 meeting.

greggles → credited benjifisher → .

mcdruid → credited benjifisher → .

@camhoward:

I guess we agree on most things, but not all.

Instead of adding weights everywhere, we get the same result if we simply revert the change from #2219393. We go back to the order of the primary tabs before Drupal 11.2, and primary tabs are listed by weight and then the order in which they are defined. That is what I did in MR !12792.

I do not see the advantage of sorting alphabetically and then adding weights to everything so that the alphabetical sort never applies.

Clarification: sorting by weight is always primary, before and after #2219393. The only question is whether the secondary sort should be alphabetical or order in which they are defined.

@godotislate:

I think the BC problems are easier to handle than you suggested in #36.

A boolean field effectively has 3 possible values: TRUE, FALSE, or NULL. When rendering the block and inspecting the value of the new config item, treat NULL as TRUE.

Someone (@alexpott) once commented that ... ?? TRUE had a bad "code sniff" or something like that. So probably add a code comment, explaining that this is for BC.

We looked at this issue again at 📌 Drupal Usability Meeting 2025-08-15 Active . (Present: benjifisher, rkoller, simohell.)

We still think that something positive ("Add a CSS class") is clearer than something negative ("Be the same on all pages"). But you make a good point about not mentioning the "active-trail class" in the UI. Can we compromise on something less specific, like one of these?

• Add a CSS class to parent links
• Highlight the ancestors of the active menu link with a CSS class
• Add a CSS class to ancestors of the current page
• Add a CSS class to links leading to [or above] the current page

The goal is that the typical user will have to read the help text (description) once, and then the label should be enough when they return to the configuration form. So the label does not have to say too much. On the other hand, the help text should explain

• This option has an effect when the block is shown on a page listed in the block.
• It applies to all "parents" or "ancestors" of the current page that are shown in the block.
• Adding the CSS class has performance implications.

It took two tries, but I added the assertions for the button text, before and after toggling.

Unless I totally misunderstand the nth-child selector, my mistake was to look for the second item in the admin menu after installing the Standard profile. That is Structure. I am not sure which profile the Nightwatch tests use, but it seems that the second item is Configuration.

Back to NR.

benjifisher → created an issue.

I am adding issue credits based on the Security Team's private notes.

benjifisher → created an issue.

How is it easier to use the \PASSWORD_* constants? Using drush php:

> password_algos()
= [
    "2y",
    "argon2i",
    "argon2id",
  ]

I think we should let people set a hashing algorithm in services.yml.

Have we considered what to do if the service parameter is invalid? Do we fall back to PASSWORD_DEFAULT? Is there a requirements check (for the status page)?

In https://git.drupalcode.org/security/24-drupal-security/-/issues/1, we discussed a problem in the 11.x branch that did not exist in any released version of Drupal core. The issues that led to that problem were reverted, so the problem will never be part of a release.

I know that someone reading that can search for reverted issues and figure out where the problem was, but the problem has been fixed so I am not worried about mentioning it on this public issue.

The question for this issue is what label to attach when closing the private issue. I chose Closed:public, but I would have liked the option Closed:outdated. Should we add such a label?

@andypost:

What transition do you mean?

Passwords should be rehashed automatically when users log in with their passwords. The Q&A for the Password Compatiblity module → apply to this situation, too.

From #24 here:

I've manually tested that changing the hashing algorithm with existing users successfully rehashes their password in the new algorithm when they next login.

@znerol:

Should we add a config and a proper UI to make these requirement checks more actionable?

I have been thinking that this is something that would be managed through settings.yml, not the UI. In fact, I like the idea of relying on something in the filesystem. Otherwise, XSS can be exploited to change the password hashing algorithm.

If you still want to provide a configuration form, then let's have a followup issue for further discussion.

@berdir:

That is a good point.

Both the commerce module and the physical module (Physical Fields) define their own field categories and put their fields under those categories. I disagree with that decision, but I do not know if I can persuade them to change.

During the usability meeting, we thought that many users would not think that "number plus unit" would be under the Number category, so we wanted to give an example to suggest that. The core fields are suitable for simple uses: in addition to the numeric value stored per field instance, the configuration can store a prefix or suffix to indicate a type of currency or a physical unit.

If we do not use price nor physical unit, in order to avoid confusion with those two contrib modules, then what else can we use as an example? I do not think that weight gets the point across. Do you have any other suggestions?

Wanted to double-check because removing "Extend" and "Collapse" was a big part of the original issue: Was the intent to keep "Extend" and "Collapse" in the names for stable9?

That is my understanding.

Any changes to the stable9 theme have the potential to break custom themes based on it, and perhaps cause automated tests to fail. We normally think in terms of the CSS in the theme, but I think it applies to JS, too.

I am removing the tag for an a11y review, thanks to @cwilcox808's comments.

I am setting the status to NW for additional test coverage (Comments #43, #44). I will try to get that done soon, but I have a 1-week vacation coming up. If I do not find time in the next two days, then it will be at least another week before I do it.

I updated MR !138. Anyone applying a patch to 3.6.2 based on the previous version of the MR would have trouble because of conflicts from 🐛 Uncaught TypeError: toolbarElement.querySelector(...) is null Active .

I am copying the list of attendees from the Security Team's private notes.

The new approach (suggested in #70) is being implemented in 📌 Remove the ability to configure the path to Composer Active .

I am updating the snippets in the issue summary to add the `visually-hidden` CSS class.

catch → credited benjifisher → .

benjifisher → created an issue.

@cwilcox808:

Thanks for the review!

I did a little research. The text-indent rule was added in #1137920: Fix toolbar on small screen sizes and redesign toolbar for desktop → . Searching that issue, I see one mention of element-invisible, the class that was later renamed to visually-hidden, in Comment #288. But I do not see any argument against using the utility class.

I updated the MR here to use visually-hidden, and I restored the change that removes the obsolete text-indent rule. As I said in #33, the stable9 theme already overrides the CSS file, so I do not have to worry about keeping that theme stable.

... probably on the inner span.label rather than the button itself

We definitely do not want to make the button itself visually-hidden. That would hide the icon.

greggles → credited benjifisher → .

benjifisher → created an issue.

benjifisher → created an issue.

The performance test (Drupal\Tests\demo_umami\FunctionalJavascript\AssetAggregationAcrossPagesTest::testFrontAndRecipesPagesEditor()) failed with the message,

Failed asserting that 445441 ( is equal to 445601 or is greater than 445601 ) and ( is equal to 446601 or is less than 446601 ).

I think @kentr already mentioned this on Slack.

This is a case where updating the test is the right thing to do. This MR reduces the size of toolbar.menu.js by 214 bytes. Before that reduction, it was in range. By design, the performance test passes unless the number of bytes changes by more than 500, so I reset it to the current value, 445441.

Thanks for the guidance, @cwilcox808. I have updated the MR based on Comment #35. (I hope I got it right.)

I think sentence case (not title case) is the Drupal standard, so I made it "Content menu", "Structure menu", etc, not "Content Menu". Would "submenu" be clearer than "menu", or would that just be an additional, unhelpful syllable?

I am updating the issue description. At the end of the Proposed resolution section, I have this:

Screen reader announcements will be along the lines of "structure menu, expanded, button" and "structure menu, collapsed, button".

Is that right?

@heddn:

I added that work-around to the issue summary. (Back when the issue-summary template included comments, it suggested adding work-arounds. I think it was under the "Proposed resolution" section.)

Any migration that is referenced by the migration_lookup or sub_process plugin is automatically added as a dependency. (See Drupal\migrate\Plugin\Migration::getMigrationDependencies().) I just checked: they are added as optional dependencies. So that should not trigger the problem in this issue.

benjifisher → created an issue.

benjifisher → created an issue.

Looking at the patch from #4, I am pretty sure that this issue was fixed (on the 3.0.x branch) in 🐛 Contributor with no first or middle name results in incorrect processing (e.g., APA format) Active .

At least, the patch here has pretty much the same effect as the change from that issue. It is possible that the symptom of the problem changed from some earlier commit, especially since (Comment #7 here) the problem could not be reproduced on the 3.0.x branch in June, but the other issue was not fixed until July.

I am setting the status back to NW for an update to the issue summary. I already added the missing elements from the standard outline. Please

1. Fill in the "Proposed resolution" section. (See below.)
2. Add screenshots to the Before and After sections under "User interface changes". Probably you can use the screenshots attached to Comment #44 or #45.

That will make it a lot easier for others to review your work on this issue.

I would still like to see some automated tests, as I said in Comment #34, but I will not insist on that. I guess the navigation module is still experimental (especially the top bar).

From the MR, I see that you

• replaced the block-size and inset-block-start calculations with fixed values 100vh and 0
• changed the top bar (in some cases) from display: block; to display: flex; and replaced margin with padding
• removed the {% if ... %} ... {% endif %} around the <aside> element in a Twig file, and removed the data-offset-top attribute.

I can see what was changed, but the issue summary should explain the intention behind those changes.

I checked the Security Team's private notes, and the credits here look correct.

benjifisher → created an issue.

Thanks for working on this issue. As I said on 📌 Properly handle that PHP bcrypt passwords are truncated to 72 bytes Active ,

• I think this issue is the right way to go. Let's make it easy to use something other than bcrypt.
• I think we should implement hook_requirements. We could do that as part of this issue or on #3536662.

@longwave mentioned #2951046 to me. I am adding that as a related issue. Once that issue gets in, we can use something like !php/const PASSWORD_ARGON2ID in services.yml.

@znerol: Thanks for your research on when argon2 is available (Comments #32-#35 on this issue). It seems that argon2 is widely available, but PHP may not update PASSWORD_DEFAULT until it is universally available (or some other replacement for bcrypt). From https://www.php.net/manual/en/password.requirements.php:

For Argon2 password hashing, either » libargon2 is required or, as of PHP 8.4.0, OpenSSL version 3.2 or later. As of PHP 7.3.0, libargon2 version 20161029 or later is required if libargon2 is used.

I think we did the right thing in Drupal 10.1, moving away from a custom implementation and adopting the PHP standard. In my opinion, the one problem is that we hard-code the PHP default values for the algorithm and options. The only way to change that is by overriding or decorating the password service.

Several of the suggestions on this issue look to me like a step in the wrong direction: using the old implementation as a fallback, or hashing long passwords (or all passwords) before passing them to bcrypt. These suggestions add complexity and move us away from the PHP standard. Let’s move forward, not backward.

The first step is to make it easy for site owners to choose any password algorithm (and options) that their version of PHP supports. So let’s move forward with ✨ Introduce kernel parameters allowing to specify password hashing algorithm and options Active . (Again, thank you @znerol!)

I think we should also implement hook_requirements, as in MR !12815 on this issue, so that we add information to the site status report. We will have to work out the details, but I agree with a warning about the 72-byte limit whenever bcrypt is in use. We can either do that as part of #3530186 or we can implement hook_requirements in this issue, and postpone it on #3530186. We should also add some documentation and link to it from the status report.

Once that is done, we can consider a followup issue to use something other than bcrypt by default (unless bcrypt is the only available algorithm).

Today's comments on 📌 Use ARIA disclosure pattern for submenu buttons in vertical toolbar orientation Needs work suggest using something other than the id attribute (and using text inside the button instead of an aria-lebelledby attribute). So maybe wait on this issue until we get a decision there.

I see. That is different from what I worked on a few years ago. There, the navigation came from the book module, and it contained the full structure of the book. So all my JS had to do was find the current page in that tree and then climb up the tree, adding the active-trail class.

I updated the MR, removing the CSS rule that is no longer needed if the button has no text (inner HTML). I remembered (barely) to update the .pcss.css files along with the .css files. I updated toolbar.icons.theme.css in the toolbar module and the claro theme.

After a little discussion on Slack, a few days ago, with @nod_, we agreed that the stable9 theme should not be changed. It already has its own version of toolbar.icons.theme.css. I added a copy of toolbar.menu.js (before any changes from this issue) to the theme, and updated the libraries-override section in its .info.yml file.

Testing: I used drush generate theme to create a sub-theme of stable9. I enabled that and set it as the admin theme. I checked that the old markup was used on admin pages and the new markup was used on non-admin pages.

When I finished that, I saw that @kentr had made some suggestions on the MR, perhaps in response to @nod_'s comment there. I think it will be easier to discuss those suggestions here instead of on the MR. From the issue summary, the current markup (11.x) for the button (when the submenu is collapsed) is

<button class="toolbar-icon toolbar-handle" style=""><span class="action">Extend</span> <span class="label">Structure</span></button>

Using the current MR, that changes to

<button aria-expanded="false" aria-labelledby="toolbar-link-system-admin_structure" class="toolbar-icon toolbar-handle" style=""></button>

I think @kentr proposes changing that to

<button aria-expanded="false" class="toolbar-icon toolbar-handle" style=""><span class="label">Structure</span></button>

That is, remove the aria-labelledby attribute and restore the text inside the button (inner HTML).

I am not an accessibility expert, so I have no opinion on that suggestion. If we can get consensus on what the markup should be, I am happy to update the MR. (Among other things: if we restore the text, then the CSS rule is no longer redundant.)

The tests are now passing, so MR !166 is really ready for review. (Well, eslint complains. But I did not touch any JavaScript files, and that test also has warnings on the 3.x branch.)

At my day job, we use the admin_toolbar module, which overrides how the menu links are rendered. The result is that they do not have id attributes, which makes the changes from this issue ineffective.

I added ✨ Menu links should have id attribute Active for the admin_toolbar module, but I am not sure it will be accepted. There may be a reason (performance?) for not including id attributes in the admin_toolbar module. In that case, is there any other value we could use here for the aria-labelledby attribute?

The toolbar_tools_menu_navigation_links() function in this module is clearly based on toolbar_menu_navigation_links() in the core toolbar module. I am not sure why the current version (in this module) does not set the id attribute. Perhaps it is intentional, for performance reasons.

I checked the Git history for core:

% git hist -S "['attributes']['id']" -- core/modules/toolbar/toolbar.module 
...
* 70bed3385fe 2014-07-30 | Issue #2301317 by pwolanin, dawehner, Wim Leers, effulgentsia, YesCT, xjm, alexpott: MenuLinkNG part4: Conversion. [Alex Pott]
* 44f76c6bcff 2014-07-30 | Revert "Issue #2301317 by pwolanin, effulgentsia, Wim Leers, dawehner, alexpott: MenuLinkNG part4: Conversion." [Alex Pott]
* fd2db9cd352 2014-07-30 | Issue #2301317 by pwolanin, effulgentsia, Wim Leers, dawehner, alexpott: MenuLinkNG part4: Conversion. [Nathaniel Catchpole]
...
* ab07a33aba4 2013-02-01 | Issue #1847198 by benjifisher, jessebeach, effulgentsia: Update the structure returned by hook_toolbar(). [webchick]
...
* 7bbf113d4ed 2012-11-21 | Issue #1137920 by jessebeach, lewisnyman, tkoleary, Bojhan, webchick, benjifisher, nod_, sjbassett, kathryn531, effulgentsia, Everett Zufelt: fixed toolbar on small screen sizes and redesign toolbar for desktop. [Dries]

So the id attribute is set in the core module at least since 2014. (The commits from 2012 and 2013 are some of my earliest contributions to Drupal.)

A similar search in this module showed no results, so it seems the id attribute has never been set:

% git hist -S "['attributes']['id']" -- admin_toolbar.module && echo done
done

benjifisher → created an issue.

1. It is pretty easy to review the comments on 📌 Menu Local Tasks should be sorted by alphabet if there is no weight set Needs review .

• 2014-03-17: The issue was created, saying that it restored the behavior in Drupal 7. (I have not checked that claim.)
• After Comment #6 (6 weeks later) and one patch (which failed automated tests) there was no activity until ...
• 2025-01-18: Comments #20 (Should we do this?) and #21: (It makes sense to me.)

There is no further discussion of whether it is a good idea and (as I said in #16 here) no discussion of translation.

2. Using my MR !12792, we do not have to edit any of the YAML files.

I count 32 YAML files:

$ ls core/modules/*/*.links.task.yml | wc -l
32

If we decide to keep the alphabetical sort, then we have to review all of these and decide whether to add weights. For example:

• config.links.task.yml: Keep the order Import, Export or switch to alphabetical?
• taxonomy.links.task.yml: Keep the order Edit, Delete or switch to alphabetical?

I do not think we need a test for each of these. The test added in #2219393 is enough.

I commented on 📌 [PP1] Refine field descriptions Active during the meeting. I just edited the comment to mention this meeting.

I see that cedewey added another comment about 15 minutes ago.

I commented on 📌 Make menu trail behaviour in SystemMenuBlock optional Active shortly after the meeting. I also left a second, technical comment suggesting a different approach. We might not need any changes to the config form after all.

This issue reminds me of some work I did several years ago. I even gave a few presentations on it. (The most recent is https://slides.benjifisher.info/caching-midcamp-2020.html#/strategy. I have to do something about the TLS certificate.) At the time, it did not occur to me to make the same change in Drupal core.

Why not use JavaScript to add the active-trail class? It is a lot simpler: JavaScript is really good at traversing the DOM, and the current PHP code is a little messy.

But code simplification is just a side benefit. The real advantage is that you can cache the navigation and still have the active-trail class.

Usability review

We discussed this issue at 📌 Drupal Usability Meeting 2025-07-25 Active . That issue will have a link to a recording of the meeting. (See the second half of the recording.)

For the record, the attendees at the usability meeting were benjifisher, rkoller, and simohell. I am adding issue credit for them.

We all agreed that the option should be reversed. The current label (title), "Show the same menu markup on every page", does not give enough information, so the user has to read the description (help text) to figure out what it means. It is sort of like trying to describe what an option does not do instead of describing what it does.

We did not have time to recommend replacement text, but start with something like

Add the active-trail class

Maybe just use that and keep further explanation in the help text. In the help text, mention the pros and cons: the theme may style the active-trail class, but it has a performance impact.

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

@Emma Horrel:

On ✨ Add the option to customize the order of field types in field type groups Active (postponed on this issue), you asked,

Following up on a suggestion to do user testing on this. To design a test I would need to know:
• Type of Drupal user likely to use this functionality (and therefore impacted by a change to the interface)
• Typical scenario in which the user would encounter this interface (e.g. when would they need/want to change the order of fields types in field type groups?)
• Desirable outcome from the scenario and problem the change in interface has addressed (e.g. we want to ensure the Drupal user can distinguish between the field types and field groups because not being able to do so would result in [what are the tangible negative consequences of the problem not being addressed])
...

For this issue,

• A site builder, creating a new site or adding features to an existing site, will use this functionality. We have to consider both experienced users and less experienced ones. Experienced users are more interested in completing the task efficiently. Less experienced users (including those who do not have a lot of technical knowledge) need more guidance in deciding which field type to use.
• The scenario where this issue comes up is when adding a new field to a content type (or a taxonomy term, paragraph type, or other entity type).
• This issue aims to help less experienced users. The updated text should help them choose the right type of field without having too much trial and error.

@rkoller has brought up this issue at meetups. Even fairly experienced users are not aware of some of the information that we have added while working on this issue. See Comments #51 and #53 on this issue.

benjifisher → created an issue.

Thank you for all the work you have done for Drupal over the years! Drupal has gained a lot from your expertise. Personally, I picked up a few pointers about accessibility (a11y) but I am certainly not an expert.

By the way, #3526266 changes markup (core/modules/navigation/templates/top-bar.html.twig) as well as CSS. Let's consider both before making further changes to the CSS.

I tested with the current 11.x and reproduced the problem.

Then I reverted 6bdfd060b136 (the commit from 🐛 Navigation top bar should utilize Drupal.dispace() Active , 2025-06-04). I re-tested, and the navigation section (admin toolbar) had full height. That is two days later than 🐛 Regression: Drupal.displace() not working on new Navigation module in 11.2 Active , so I think it is fair to say that #3526266 caused this bug, confirming Comment #28. I am adding #3526266 as a related issue.

I am setting the status to NW for some automated tests. Otherwise, we will have to go through the same arguments and similar research the next time this feature gets broken. I notice the comment #3526180-8: Regression: Drupal.displace() not working on new Navigation module in 11.2 → :

Note that I don't test out displace's functionality (that gets tested in its respective tests), I just test that the attribute is added properly, which is what the JS in this module does.

This time, can we get a functional test? That is, figure out the various combinations of window size, top bar, and navigation area, and confirm that things fit together the way we want them to?

@finnsky:

The top bar was added in a hurry and we need to review how it works with displace

Is there already an issue for that? If so, it would help to add it here as a related issue.

I am working on the same project as @cmah.

On our site, the font family is saved as an empty string in the database:

mysql>  SELECT entity_id, field_hwp_icon_icon, field_hwp_icon_family, field_hwp_icon_classes FROM paragraph__field_hwp_icon;
+-----------+---------------------+-----------------------+------------------------+
| entity_id | field_hwp_icon_icon | field_hwp_icon_family | field_hwp_icon_classes |
+-----------+---------------------+-----------------------+------------------------+
|         4 | cabin               |                       | NULL                   |
|         6 | 30fps               |                       | NULL                   |
|         7 | phone_android       |                       | NULL                   |
+-----------+---------------------+-----------------------+------------------------+
3 rows in set (0.00 sec)

I am not sure why that is. We are using the paragraphs and paragraphs_layout modules, and some custom code. But this did not cause problems with Version 2.0.1, so I consider it a bug in this module. (Perhaps it is a bug that is only exposed when there are bugs in other parts of the system.)

I am updating the "Steps to reproduce" in the issue summary.

@kentr:

I confirmed what you said about toolbarApiTest.js, and I agree it looks wrong. But I do think it is out of scope for this issue. Also, I am not sure what the difference is between that test and toolbarTest.js: what does "Tests of the existing Toolbar JS Api." mean? So let’s leave that for a separate issue.

I added a few lines to toolbarTest.js, checking that the aria-expanded attribute changes as expected. I did not check the aria-labelledby attribute, nor that the inner HTML of the <button> element is empty. I can test either or both of those if anyone thinks it is needed.

I also updated the "User interface changes" section of the issue summary.

I am leaving the status at NW for two reasons:

1. I think the "Proposed resolution" needs to be updated. The current version mentions "Use a constant name for the button:"parent-link-title sub-menu"." That is not what the current MR does. I also added the issue tag for that.
2. There is a CSS rule in toolbar.icons.theme.css (in the toolbar module and in two of the themes) that sets text-indent to a large, negative number for the body of the <button> element. Now that the body is empty, I think we can remove that rule.

@AbhijithS:

Thanks for adding the test. It is a very nice test! Just to be sure, I hacked the test in a few ways:

• Search for "Foo Edit" instead of "Edit". The test fails as expected.
• Change assertNotFalse() to assertTrue(). The test fails because it is comparing an int to a bool.

I have just two suggestions:

1. Remember to update the issue status (from NW to NR) when appropriate.
2. I suggest making the positive assertion assertIsInt() instead of the negative assertion assertNotFAalse().

As I said in Comment #16, I think a better solution is to revert the commit from 📌 Menu Local Tasks should be sorted by alphabet if there is no weight set Needs review :

1. That issue changed the order of primary tabs on at least one other admin page besides the term-edit page. We should fix all of them at once.
2. That issue did not consider translations. Neither choice (sort before translating or sort after translating) really works.

That decision may be controversial, so instead of updating the existing MR, I added a new one. I kept the test from MR 12697 (with the change I suggested earlier). Instead of adding weights to the YAML files, I reverted the commit from #2219393.

I updated the issue summary, explaining the different approaches in the two merge requests. I am also running the test-only job on MR 1297.

benjifisher → created an issue.

Using git bisect, I found the issue that caused this problem: 📌 Menu Local Tasks should be sorted by alphabet if there is no weight set Needs review . Having found that issue, it seems clear that it caused the problem in this issue. I am adding it as a related issue.

As I said in my previous comment, the taxonomy-term pages might be only part of the problem. Looking through the *.links.task.yml files, I notice that the config module defines tabs (local tasks) on /admin/config/development/configuration: Import, Export in Drupal 11.1 and Export, Import in Drupal 11.2. Is one choice better than the other? Should we add weights to restore the order in earlier versions of Drupal or should we sort them alphabetically?

Instead of fixing just the taxonomy-term page, I think it would be better to expand the scope of this issue to review all the *.links.task.yml files, and add weights on a case-by-case basis.

But I think there is an even better solution: revert the changes from #2219393.

On that issue, there is no discussion of translation and how it affects the order of the tabs (local tasks).

The current behavior (in 11.2, after #2219393) is to sort alphabetically before translating. For the taxonomy-term page, that means the order is

• English: View, Delete, Edit, Revisions, Translate
• Spanish: Ver, Eliminar, Editar, Revisiones, Translate

The effect for Spanish users is that the the tabs have been sorted alphabetically based on the English translations, which is not helpful.

Yes, we can fix this page (as the current MR on this issue does). But the same fix would be required in many places: core, contrib, and custom modules.

Another option is to fix #2219393 so that it sorts after translation. But that is also bad. It would mean that people looking at the site in different languages would see the tabs in different orders.

Neither option seems good to me. I vote to revert #2219393.

Thanks to all for your work on this issue.

Since this issue is a bug report, it should have an automated test to make sure that, once we fix it, it does not return. I am setting the status back to NW for that, and adding the issue tag.

@sagarsingh24:

Thanks for testing the changes, and for providing screenshots. I am adding them to the issue summary (under "User interface changes") and marking your account as Approved. Welcome to the Drupal community!

Perhaps you expected issue comments to support markdown formatting. They do not, but "soon" we will use GitLab issues, which do. (I hope this happens before the end of 2025.)

The "R" and "T" in RTBC stand for Reviewed and Tested. From your comment, it looks as though you tested, but you did not say anything about reviewing the code changes.

@oily:

If you make changes to a merge request (MR) after it has been reviewed and tested, please set the status back to NR. In this case, you could have taken the reviewer role instead of making changes yourself: ask for the updates, set the status to NW, and then review the new version after someone else had updated the MR.

All:

Before agreeing to these changes, I would like to know what changed between 11.1.8 and 11.2.2 to cause this problem. Without that investigation, we might be fixing a symptom of a problem instead of the underlying problem, or we might be fixing only part of a larger problem.

@snehalgaikwad:

It looks as though you started to work on a merge request (MR) but did not commit any changes.

Before you do any more work on this issue, please review the comments. In #15, @aaronmchale indicated that this issue needs an update to the issue summary and a rescope. At this point, we have to decide what to do. It is too early to start implementing anything, unless you have your own idea that you would like others to test.

@kentr:

Thanks for the reply (here and on the related issue).

I will try to find time to add some tests coverage for this issue. From the comments, that is the only thing holding it back. (The Remaining tasks list "Update JS + templates". I think that is already done, although the current MR changes only one JS file, no templates. If I have that right, can someone that item?)

I know nothing about ARIA recommendations, but on #3286466-50: Tabbing order does not satisfy 508 accessibility requirements → , @rkoller suggested using aria-pressed instead of aria-expanded:

... But i wonder if aria-expanded is the right choice for a toggle button. I always have problem as a sighted user understanding the current state for toggle buttons that change their label inbetween states. A point that Leonie Watson also illustrated in their talk for smashing magazine: https://youtu.be/OUDV1gqs9GA?t=3222 . She advocated to use aria-pressed instead. That way the button state is either pressed/selected or not and the button label remains the same between states. That way things are completely clear and unambiguous for screen reader users?

Does that apply here? Should we consider it?

@andrewmacpherson: It has been more than 5 years since you self-assigned this issue and last commented. I am un-assigning it now.

Oddly, this issue does not show up when I search for "aria-expanded". Maybe that is because it appears inside a code tag.

If so, then it should show up now in the search.

benjifisher → created an issue.

catch → credited benjifisher → .

catch → credited benjifisher → .

I am copying the list of attendees from the Security Team's notes.

benjifisher → created an issue.

I put in some sort of work-around for my day job, but I have not had a chance to figure out where the problem is.

If this issue stays open, then I am more likely to make time to look at it eventually, but I will not complain if you close it as "cannot reproduce".

@baikho:

Thanks for the additions to the issue summary. That should make testing a lot easier. It may be a week or two before I have time to test, but maybe someone else will pick it up before I do.

Also beyond the error message issue, ...

That looks like it could be a separate issue. Maybe the SkipRow exception should be re-thrown as a SkipProcess exception. Or maybe we can stop using exceptions after 📌 Allow process plugins to flag a row to be skipped Needs work is fixed.