🇺🇸United States @benjifisher

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.

I added a comment to 📌 Improve clarity of Layout Builder "Revert to defaults" confirmation form Active , even though one of the points I made is that we think that issue should be closed and combined with ✨ Improve clarity of Layout Builder "Revert changes" confirmation form Active .

We discussed this issue and ✨ Improve clarity of Layout Builder "Revert changes" confirmation form Active at 📌 Drupal Usability Meeting 2025-07-04 Active . That issue will have a link to a recording of the meeting. For the record, the attendees at today's usability meeting were @benjifisher, @rkoller, and @simohell.

We agree with Comment #16: this issue should be combined with #3528251. @danielveza, your own Comment #20 suggests that the current MR for this issue is not a good solution (and we agree). That seems at odds with your reasons in Comment #17 for not combining the issues.

We did a little testing, and we can confirm that inline blocks (Content blocks created in Layout Builder) are deleted from the database if you Discard changes. If you have inline blocks saved in a custom layout, and you Revert to defaults, then they are still in the database but you cannot use them nor even edit them. (The page at `/admin/content/block` is a View. By default, it filters on Reusable: True. It is easy to edit that view and expose that filter, and then you can list the inline blocks.)

We did not settle on recommended text for these forms, but we can suggest some guidelines:

1. Use the description text instead of a long title. (We agree with Comment #20.)
2. Do not repeat things in the title and the description (as pointed out in Comment #20).
3. Shorten the existing title. Make it a statement, not a question, consistent with the choices Confirm/Cancel. We would like something like "Revert to the default layout".
4. Give context about which page is affected. Think of the content editor who gets interrupted, or is editing the layout of several pages in different tabs.

We also noticed a few problems that are not in the scope of this issue. The Umami demo profile does not display the confirmation form well. It would be nice to implement #1842036: [META] Convert all confirm forms to use modal dialog → .

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

xjm → credited benjifisher → .

NW for a code comment. (See my long-winded explanation on the MR.)

I am adding the tag for a followup (also in my MR comment). I will create the followup issue unless you do it first.

benjifisher → created an issue.

@baikho:

Thanks for working on this issue!

I would like to have more complete steps to reproduce (STR).

Technically "Attempt to do an entity lookup which has no entity to resolve to" is not enough. I am pretty sure that the migration lookup (not an entity lookup) has to fail, and then there has to be an exception when trying to create the stub.

The bigger problem is that I want to know how to test and see what difference the change makes in the context of an actual migration. The update to the unit test only confirms that the re-thrown exception has the expected changes. It does not show me what actually gets logged in the message table.

Good STR would include at least one migration YAML file, and probably two or three. The migration(s) could use the embedded_data source plugin. The migration(s) that trigger(s) the new code should contain rows that cover all possibilities:

• The lookup succeeds.
• The lookup fails, but the stub gets creates.
• The lookup fails, and the stub throws an exception.

benjifisher → created an issue.

On Slack, @DanielVesa suggested these issues for the next meeting:

• ✨ Improve clarity of Layout Builder "Revert changes" confirmation form Active
• 📌 Improve clarity of Layout Builder "Revert to defaults" confirmation form Active

My team tested this upgrade, and it looks good:

I just tested by downloading publications from ... in RIS format.

For example, [file in ris format]

So far, import, export, and rendering of the publications are working as expected. During the import, the RIS tags were correctly mapped to their corresponding fields (e.g., title, keywords, publisher, publication date), consistent with how it worked before the update.

I've tested with 10 publications so far.

This covers the testing (T) and I reviewed the changes (R) in Comment #6, so RTBC.

@subarnap:

The release notes for Drupal 11.0.12 ( https://www.drupal.org/project/drupal/releases/11.0.12 → ) already mention this module. Release notes are the usual way to communicate this sort of change.

Do you have any suggestion for where to communicate this in the upgrade process in addition to the release notes?

If so, is that a task for this module or would it have to be done in Drupal core?

I commented on 📌 Disallow dangerous filenames e.g. command injection characters Active .

@mcdruid:

I am sorry for the delay in adding a comment. As you said in Comment #26, the Usability team looked at this issue on 2025-05-30. I just reviewed the discussion, and your summary in #26 looks correct.

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

Thanks for clarifying that the change will not affect new uploads. In fact, the change to the whitespace default will only affect newly installed sites (or sites that newly install the file module).

At the meeting, we had the impression that stripping punctuation was non-negotiable, so the only change we fully considered was the default handling of whitespace.

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

larowlan → credited benjifisher → .

Usability review

Issue summary updates

I added the tag for an issue summary update. The purpose of the issue summary is to help anyone reviewing the issue understand the proposed changes, and the current summary does not do a good job of that.

1. The Problem/Motivation should be something like this: "When a user uploads a file to a Media reference field, there is not enough context to tell the user what kind of file is expected (audio, image, etc.)." The current text is basically the same as the Proposed Resolution.
2. The issue summary should be very clear about what the "source field" means: the file field on the media entity. This detail is mentioned in passing under the "User interface changes" section, but it should be more prominent and more explicit. This detail is important for understanding the Proposed Resolution.
3. The Steps to reproduce (STR) should be more explicit, so that they lead to a consistent state. The Umami demo profile is good for this sort of thing. My STR are typically something like this:
   
   1. Install Drupal with the Umami demo profile.
   2. Edit field_media_document at Administration > Structure > Media types > Edit Document > Manage fields (/en/admin/structure/media/manage/document/fields). For testing purposes, add something under "Help text".
   3. Edit field_media_image at Administration > Structure > Content types > Article > Manage fields (/en/admin/structure/types/manage/article/fields). Under Media type, select all options.
   4. Create a new Article node (/en/node/add/article). Use the "Add media" button to open the modal editing window, and select Document from the vertical tabs.
4. The screenshots in the User interface changes section should be based on the last step of the STR, and they should show the Help text (description) added in the earlier step.
5. Some part of the issue summary, perhaps Problem/Motivation, should compare the file upload field that does not have enough context to the form at /en/media/add/document, which does have more context (the field label and description mentioned in this issue). This comparison will help avoid confusion, point out where the label and description are currently shown, and point out the inconsistency between the two forms.

Proposed resolution

There were only two people present at the Usability meeting, so these recommendations have less weight than if it had been a larger meeting.

We considered various goals of the proposed changes:

1. Provide better context for the user uploading a file.
2. Be more consistent between the forms at /en/node/add/article and /en/media/add/document.
3. Avoid having more interface text than necessary. ("Less is more.")

The current proposal helps with (1), but goes in the wrong direction for (2). Furthermore, we felt that adding the description (Help text) to the existing field description made it easy to miss.

Instead of replacing the standard "Add field" label with the source field’s label, we feel that a better solution is to add the source field’s label and description above the add-file widget. Explicitly, we suggest something like the following:

Source field label (probably as an <h2> element) Example: Document

Source field description (maybe as a <p> element) Example: This is the help text for the Document field (for the sake of example).

Add file

...

We feel that this provides better context, since the description, if any, is more prominent. (To be clear, less is more: the default description is empty, and site builders should leave it that way unless there is something useful to put there.) We feel this increases consistency between the two forms, since the other one already has the field label as the summary of a <details> element. The drawback is that our suggestion fails the "less is more" test, but we feel that the other points make up for that.

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

0️⃣ Who is here today?

1️⃣ What should we talk about today? Suggest topics here and I will add threads. I will also check for comments on the issue for today's meeting.

2️⃣ Action items. To be added later.

3️⃣ Statistics

4️⃣ Comment in this thread if you are looking for ways to contribute. Give us some idea of what you would like to do: documentation, code review, testing, project management, ...

5️⃣ Previous minutes.

6️⃣ Announcements

7️⃣ Migrating reference fields: target_bundles may never be empty array

8️⃣ Remove memory management from MigrateExecutable

We discussed this issue at 📌 Drupal Usability Meeting 2025-06-27 Active . That issue has a link to a recording of the meeting. For the record, the attendees at the usability meeting were @benjifisher and @rkoller.

I plan to leave another comment summarizing that meeting, but until then you can watch the recording.

I watched the recording at 2x speed and did my best to note everyone who participated.

xjm → credited benjifisher → .

xjm → credited benjifisher → .

benjifisher → created an issue.

benjifisher → created an issue.

How to test

The only change in the MR for this issue is to composer.json, so it takes some work to get Composer to apply the change.

I added thetesting repository to composer.json in my project:

    "repositories": {
        "testing": {
            "type": "git",
            "url": "https://git.drupalcode.org/issue/bibcite-3522242"
        },
        "drupal": {
            "type": "composer",
            "url": "https://packages.drupal.org/8"
        },
    }

Then I ran this from the command line:

ddev composer require -W "drupal/bibcite:dev-3522242-technosophoslibris-is-abandoned as 3.0.x-dev"

I also had to remove some patches that no longer apply. I hope that is because the related issues have been fixed.

The package is used only in the bibcite_ris submodule. Luckily, that is enabled on my team's sites, so we should be able to test it.

I notice these lines in modules/bibcite_ris/src/Encoder/RISEncoder.php:

  public function decode($data, $format, array $context = []): mixed {
    /*
     * Workaround for weird behavior of "LibRIS" library.
     *
     * Replace LF line ends by CRLF.
     */
    $data = str_replace("\n", "\r\n", $data);

    $config = \Drupal::config('bibcite_entity.mapping.' . $format);
    $fields = $config->get('fields');
    $ris = new RISReader();
    $ris->parseString($data);
    $records = $ris->getRecords();

    // Workaround for weird behavior of "LibRIS" library.
    foreach ($records as &$record) {
      // ...

From a quick look at the code, it is not obvious how much of that loop is meant as a workaround.

We should investigate whether those workarounds are still needed, or if they actually cause trouble, with the new package. If they do not cause trouble, then we could defer that to a followup issue.

why checking for Drupal version?

Just for consistency. If I were writing it from the start, I would only check for whether the route exists.

@alexpott:

I created the child issue 📌 Never bypass validation when saving a config object Active .

Using the code in the MR, the testing steps with drush php lead to an exception:

> $ce->save(TRUE)

   Drupal\Core\Config\UnsupportedDataTypeConfigException  Invalid data type for config element dblog.settings:foo.

benjifisher → created an issue.

The code change is simple and does what I suggested in the issue summary: that is the R in RTBC.

I will set up a test site and ask someone on my team to do some testing. I am assigning this issue to myself, but if I do not follow up within a week, then feel free to re-assign.

@avpaderno:

Thanks for taking care of this.

I updated https://www.drupal.org/sa-contrib-2019-077 → . That should fix the problem. (I am a member of the Security Team, so I have permission to edit security advisories.)

Maintainers: please credit

• sboden, for reporting the issue
• orkut murat yılmaz, for providing detail
• Luke Stewart, for bringing this issue to my attention
• drumm, for advising on how to resolve it

I am setting the status back to NW. See my comments on the MR.

The merge request MR 2 is ready for review.

The first commit is enough to fix the problem. The second commit is just code cleanup: it should make the code a tiny bit more efficient and a little simpler but have no effect on functionality.

benjifisher → created an issue.

Just to be sure I understand the comments:

Drupal\Core\Config\Config::save() calls validateValue() (both directly and indirectly, via castValue()). That ensures that proper use of the config system protects against serializing anything other than nested arrays of scalar types.

  public function save($has_trusted_data = FALSE) {
// ...
    if (!$has_trusted_data) {
      if ($this->typedConfigManager->hasConfigSchema($this->name)) {
        // Ensure that the schema wrapper has the latest data.
        $this->schemaWrapper = NULL;
        $this->data = $this->castValue(NULL, $this->data);
      }
      else {
        foreach ($this->data as $key => $value) {
          $this->validateValue($key, $value);
        }
      }
    }

By "proper use", I mean that save() is not called with the optional $has_trusted_data parameter set to TRUE.

I am a little worried that some custom or contrib code might use $config_object->save(TRUE) in order to bypass this validation. Am I being too pessimistic?

benjifisher → changed the visibility of the branch 3.x to hidden.

benjifisher → created an issue.

I did not notice this issue before today, or I would have commented earlier.

I am not sure that we should add a new permission for rebuilding permissions.

First, the administer nodes permission is already marked as restricted. It should not be granted to "normal mid-level content users" (to use the phrase from the issue summary). Instead of introducing a new, restricted permission I think we should encourage site owners to be more careful about granting the existing one.

Along with https://www.drupal.org/sa-core-2025-002 → , I released the Granular Node Permissions → module. That provides permissions suitable for less-privileged roles, so that common tasks can be done without the administer nodes permission. There is also an issue to add one of those permissions to core: ✨ Publish nodes permission Postponed: needs info . The goals of the contrib module and the core issue are the same: make it easier to get work done without the restricted permission, so that site owners can be more stingy about granting it.

benjifisher → created an issue.

I am updating some formatting and assigning issue credits.

If it were just the formatting, then I would mark the issue Fixed. Since I am also assigning credit, I would like someone else to confirm that.

@ddavisboxleitner:

Thanks for the transcript. Next time, remember to update the issue status.

I am adding a transcript.

I am adding a transcript.

Oops, I got the date wrong.

I am adding credit for the users mentioned in Comment #51.

I added a couple of commits, implementing field_type_category_info_alter:

1. The additional text for the file_upload group should only appear when the media module is enabled, so it cannot be in a static YAML file. It also needs a link to the help page if the help module is enabled.
2. At the usability meeting, we agreed that it would be good to add a link to the summary text for the formatted_text group. The most reliable way to generate the link is with PHP code, using the route name instead of the path.

I am adding credit for the users mentioned in Comment #53.

Clarification: one of the points in Comment #54 is

Enable the addition of links to the summary line in yaml files

That refers to text like the following:

Formatted text fields allow HTML markup. The field can be configured to allow one or more of the configured text formats.

It would be helpful to add a link to /admin/config/content/formats (the filter.admin_overview route). Unfortunately, it is technically difficult to add that link since the text is part of core/modules/text/text.field_type_categories.yml.

benjifisher → created an issue.

xjm → credited benjifisher → .

xjm → credited benjifisher → .