🇺🇸United States @benjifisher

Would it make sense to put the current list of tests with intermittent failure into a special test group for GitLab CI? It would require some maintenance, but I think it would save time in two ways:

1. When a test fails, I do not have to come to this issue and see whether it has been reported. I will know from the GitLab UI that re-running the test is the right thing to do.
2. When I re-run the test, I will not have to re-run all the well-behaved tests in the existing test group.

I plan to continue the discussion from the 2025-10-10 meeting. See the long comment there.

I suggested this on Slack, and @rkoller agreed. Let's get some other opinions before making this change.

Looking again at the telephone field, I think we should strike "Store" from

> Store a telephone number

But then we are left with just "a telephone number". Usually, if the description does not say any more than the label, it is best to leave the description off.

Perhaps we should say something like this instead:

> A telephone number, stored as a simple string.

That will clarify that Drupal does not store it as any sort of complex data structure. It does not store country code, area code, or anything else. That is kind of unfortunate, since it limits what you can control when displaying it.

I do not see anything in the screenshots (the original nor the updated one) to indicate which fields are required. Am I missing something?

I generally prefer to put admin paths in <code> tags. In this case, it is especially helpful since the unknown (and unclosed) <content_type> tag was being stripped.

We discussed this issue at 📌 Drupal Usability Meeting 2025-10-10 Active . That issue has a link to a recording of the meeting. I am giving issue credit here to the attendees at the usability meeting: benjifisher, jannafiester, joannajackson, rkoller, and simohell.

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

In preparation for the meeting, I went through all the text changes suggested or requested by dww, in the order they appear on the MR. I figured out where they appear in the UI and prepared the list below (copied from #3550127). During the meeting, we got through about half of the list: we got as far as Media. We will update the MR based on the discussion, but I do not plan to reply to each point on the MR. @dww, If you still object or have questions, then please either bump the thread (i.e., add another comment) or look at the recording. Or maybe it is simpler if you just resolve the threads where you are satisfied with our choice, and then I will reply to the unresolved threads.

@dww reviewed the MR for 📌 [PP1] Refine field descriptions Active . The review included some technical points and some suggestions for improving the interface text. The following sections summarize those suggestions. Each numbered list includes

1. The text in 11.x.
2. The text in the current MR.
3. The suggestion or comment from @dww.

File Upload > File

1. • For uploading files
   • Can be configured with options such as allowed file extensions and maximum upload size
2. • Downloadable files
   • Configure allowed file extensions, maximum upload size, and storage location
   • Examples: document (pdf, txt), audio (mp3), video (avi, mov), image (jpeg, png, webp, gif), etc.
3. Given we have a separate Image field type, why does the help text for this specifically recommend using a generic File to upload image file types?

Date and Time

1. Field to store date and time values.

• A date and/or time value
• The field types use different formats to store date and time in the database. All field types can be displayed in various ways using the Manage display tab.’

1. The 2nd sentence here is true of all fields, not just date_time, right? Do we want this here?

Formatted Text > Long text with summary

1. • Ideal for longer texts, like body or description with a summary
   • Allows specifying a summary for the text
   • Supports long text without specifying a maximum length
   • May use more storage and be slower for searching and sorting
2. • Extends Long text
   • Uses separate text areas (multiple rows) for input and summary
   • Summary defaults to a trimmed value of the long text
   • Recommended for content types that will be displayed in cards or teasers
3. I thought we’re trying to deprecate this field type. 😅 Not recommended for anything? 😆

I do not think this is a serious suggestion.

Telephone Number

To see this option, enable the telephone module.

1. This field stores a telephone number.
2. Store a telephone number with an output link option
3. Store a telephone number with an option to output as a link

Same concern as with email about what an "output link option" is.

Selection List > Text

1. • Values stored are text values
   • For example, ‘US States’: IL => Illinois, IA => Iowa, IN => Indiana
2. • Values (machine names) are text strings
   • By default, the Value is based on the Name
   • Example Name (Value): Apple (apple), Mango (mango), Black Cherry (black_cherry)
   • Example Name (Value): Most (0.9), Half (0.5), Some (0.3)
3. • Example: Apple (apple), Mango (mango), Black Cherry (black_cherry)

I know "0.9" is technically a "text string", but this seems confusing.

Selection List > Integer

The Name (Value) part of this is a bit confusing. Maybe just:

1. • Values stored are numbers without decimals
   • For example, ‘Lifetime in days’: 1 => 1 day, 7 => 1 week, 31 => 1 month
2. • Values are whole numbers
   • Example Name (Value): Disagree (-1), Neutral (0), Agree (1)
3. • Example: Disagree (-1), Neutral (0), Agree (1)

Reference > Content

1. An entity field containing an entity reference.
2. • Link content, such as Basic page or Article
   • Examples: related articles, next/previous links
3. • Link content

These node types may or may not exist on any given site. Is this a good idea?

Media

1. • Field to reference media. Allows uploading and selecting from uploaded media.
2. • Create media type with the ability to upload or link assets Maybe just this:
3. • Reference a media item, either a new upload or link existing

Date and Time

1. • Ideal when date and time needs to be input by users, like event dates and times
   • Date or date and time stored in a readable string format
   • Easy to read and understand for humans
2. • Human-readable string format
   • Input requires: date
   • Example: 2001-01-14T00:00:00
3. This is called a "Datetime", but "Input requires: date"? That’s confusing. Plus the example shows a time. What value is "Input requires: date" adding here? Less is more? Remove it?

Plain Text > Long Text

1. • Ideal for longer texts, like body or description
   • Supports long text without specifying a maximum length
   • May use more storage and be slower for searching and sorting
2. • Uses a text area (multiple rows) for input
   • No fixed maximum length
   • May use more storage and be slower searching and sorting
   • Recommended for longer texts, like body
3. I thought we’re generally moving away from a "body" field in core. Do we even need this line at all? This option is labeled "Long text". Doesn’t that imply it’s recommended for long text?

Number > Integer

1. • Number without decimals
   • For example, 123
2. • Whole numbers
   • Values are positive, negative, or zero
   • Maximum values depend on the system
   • Examples: 123, -123, 0, 3 (pi)
3. • Examples: 123, -123, 0

3 != pi. 🤓 😆 This is a weird example.

Isn’t the 2nd line ("Values are positive, negative, or zero") a definition of the 1st ("Whole numbers")? Do we need to say both?

Sorry, just double checked myself. "Whole number" is defined as a positive integer or 0.

Negative integers are not whole numbers. This new help text is definitely wrong. Perhaps the original text "Numbers without decimals" is the shortest, most clear.

@rkoller:

looking at https://www.britannica.com/dictionary/eb/qa/what-is-an-integer how about a small addition to yours and replace "Whole number" with "Number without decimal parts or fractions" instead?

Number > Float

In order to see this option, you have to remove the line

  no_ui: TRUE,

in core/lib/Drupal/Core/Field/Plugin/Field/FieldType/FloatItem.php.

1. • In most instances, it is best to use Number (decimal) instead, as decimal numbers stored as floats may contain errors in precision
   • This type of field offers faster processing and more compact storage, but the differences are typically negligible on modern sites
   • For example, 123.4 km when used in imprecise contexts such as a walking trail distance
2. • Numbers with decimal parts and exponents
   • Only limits provided by the system
   • Examples: 1.23, -1.23, 0.00e1, 1e100, 6.02214076e23
3. I don’t know what "Only limits provided by the system" means, using either new nor "old" eyes. 😆

Reference > ???

You only see this default value if a module declares a reference type to be "common" but does not provide a description. For testing purposes, you can remove the description from core/modules/node/src/Hook/NodeFieldHooks.php.

1. • An entity field containing an entity reference.
2. • A reference to a(n) @item
3. I know we don’t know what @item will be, but a(n) is a bit awkward. Again, not sure what to suggest.

Reference > Other

1. • An entity field containing an entity reference.
2. • Choose from all available options on the next screen
   • Any content or configuration entity could be referenced
3. Again, with "new eyes", WTF is a "configuration entity"? 😅 Not sure what to suggest here.

@rkoller:

we’ve simply used the terminology that is used in the next step on the field settings. if you take a look at the "type of item to reference" select then you see that the options are grouped by "content" and config". that was the reasoning why we went with "…content or configuration entity…"

Email

1. • Field to store an email address.
2. • An email address with an output link option
3. • An email address with an option to output as a link

I know we’re attempting to be terse, but trying to put on my "new eyes", I’m not sure I’d understand what "an output link option" is.

benjifisher → created an issue.

@dww reviewed the MR for 📌 [PP1] Refine field descriptions Active . The review included some technical points and some suggestions for improving the interface text. The following sections summarize those suggestions. Each numbered list includes

1. The text in 11.x.
2. The text in the current MR.
3. The suggestion or comment from @dww.

File Upload > File

1. • For uploading files
   • Can be configured with options such as allowed file extensions and maximum upload size
2. • Downloadable files
   • Configure allowed file extensions, maximum upload size, and storage location
   • Examples: document (pdf, txt), audio (mp3), video (avi, mov), image (jpeg, png, webp, gif), etc.
3. Given we have a separate Image field type, why does the help text for this specifically recommend using a generic File to upload image file types?

Date and Time

1. Field to store date and time values.

• A date and/or time value
• The field types use different formats to store date and time in the database. All field types can be displayed in various ways using the Manage display tab.’

1. The 2nd sentence here is true of all fields, not just date_time, right? Do we want this here?

Formatted Text > Long text with summary

1. • Ideal for longer texts, like body or description with a summary
   • Allows specifying a summary for the text
   • Supports long text without specifying a maximum length
   • May use more storage and be slower for searching and sorting
2. • Extends Long text
   • Uses separate text areas (multiple rows) for input and summary
   • Summary defaults to a trimmed value of the long text
   • Recommended for content types that will be displayed in cards or teasers
3. I thought we’re trying to deprecate this field type. 😅 Not recommended for anything? 😆

I do not think this is a serious suggestion.

Telephone Number

To see this option, enable the telephone module.

1. This field stores a telephone number.
2. Store a telephone number with an output link option
3. Store a telephone number with an option to output as a link

Same concern as with email about what an "output link option" is.

Selection List > Text

1. • Values stored are text values
   • For example, ‘US States’: IL => Illinois, IA => Iowa, IN => Indiana
2. • Values (machine names) are text strings
   • By default, the Value is based on the Name
   • Example Name (Value): Apple (apple), Mango (mango), Black Cherry (black_cherry)
   • Example Name (Value): Most (0.9), Half (0.5), Some (0.3)
3. • Example: Apple (apple), Mango (mango), Black Cherry (black_cherry)

I know "0.9" is technically a "text string", but this seems confusing.

Selection List > Integer

The Name (Value) part of this is a bit confusing. Maybe just:

1. • Values stored are numbers without decimals
   • For example, ‘Lifetime in days’: 1 => 1 day, 7 => 1 week, 31 => 1 month
2. • Values are whole numbers
   • Example Name (Value): Disagree (-1), Neutral (0), Agree (1)
3. • Example: Disagree (-1), Neutral (0), Agree (1)

Reference > Content

1. An entity field containing an entity reference.
2. • Link content, such as Basic page or Article
   • Examples: related articles, next/previous links
3. • Link content

These node types may or may not exist on any given site. Is this a good idea?

Media

1. • Field to reference media. Allows uploading and selecting from uploaded media.
2. • Create media type with the ability to upload or link assets Maybe just this:
3. • Reference a media item, either a new upload or link existing

Date and Time

1. • Ideal when date and time needs to be input by users, like event dates and times
   • Date or date and time stored in a readable string format
   • Easy to read and understand for humans
2. • Human-readable string format
   • Input requires: date
   • Example: 2001-01-14T00:00:00
3. This is called a "Datetime", but "Input requires: date"? That’s confusing. Plus the example shows a time. What value is "Input requires: date" adding here? Less is more? Remove it?

Plain Text > Long Text

1. • Ideal for longer texts, like body or description
   • Supports long text without specifying a maximum length
   • May use more storage and be slower for searching and sorting
2. • Uses a text area (multiple rows) for input
   • No fixed maximum length
   • May use more storage and be slower searching and sorting
   • Recommended for longer texts, like body
3. I thought we’re generally moving away from a "body" field in core. Do we even need this line at all? This option is labeled "Long text". Doesn’t that imply it’s recommended for long text?

Number > Integer

1. • Number without decimals
   • For example, 123
2. • Whole numbers
   • Values are positive, negative, or zero
   • Maximum values depend on the system
   • Examples: 123, -123, 0, 3 (pi)
3. • Examples: 123, -123, 0

3 != pi. 🤓 😆 This is a weird example.

Isn’t the 2nd line ("Values are positive, negative, or zero") a definition of the 1st ("Whole numbers")? Do we need to say both?

Sorry, just double checked myself. "Whole number" is defined as a positive integer or 0.

Negative integers are not whole numbers. This new help text is definitely wrong. Perhaps the original text "Numbers without decimals" is the shortest, most clear.

@rkoller:

looking at https://www.britannica.com/dictionary/eb/qa/what-is-an-integer how about a small addition to yours and replace "Whole number" with "Number without decimal parts or fractions" instead?

Number > Float

In order to see this option, you have to remove the line

  no_ui: TRUE,

in core/lib/Drupal/Core/Field/Plugin/Field/FieldType/FloatItem.php.

1. • In most instances, it is best to use Number (decimal) instead, as decimal numbers stored as floats may contain errors in precision
   • This type of field offers faster processing and more compact storage, but the differences are typically negligible on modern sites
   • For example, 123.4 km when used in imprecise contexts such as a walking trail distance
2. • Numbers with decimal parts and exponents
   • Only limits provided by the system
   • Examples: 1.23, -1.23, 0.00e1, 1e100, 6.02214076e23
3. I don’t know what "Only limits provided by the system" means, using either new nor "old" eyes. 😆

Reference > ???

You only see this default value if a module declares a reference type to be "common" but does not provide a description. For testing purposes, you can remove the description from core/modules/node/src/Hook/NodeFieldHooks.php.

1. • An entity field containing an entity reference.
2. • A reference to a(n) @item
3. I know we don’t know what @item will be, but a(n) is a bit awkward. Again, not sure what to suggest.

Reference > Other

1. • An entity field containing an entity reference.
2. • Choose from all available options on the next screen
   • Any content or configuration entity could be referenced
3. Again, with "new eyes", WTF is a "configuration entity"? 😅 Not sure what to suggest here.

@rkoller:

we’ve simply used the terminology that is used in the next step on the field settings. if you take a look at the "type of item to reference" select then you see that the options are grouped by "content" and config". that was the reasoning why we went with "…content or configuration entity…"

Email

1. • Field to store an email address.
2. • An email address with an output link option
3. • An email address with an option to output as a link

I know we’re attempting to be terse, but trying to put on my "new eyes", I’m not sure I’d understand what "an output link option" is.

@ultimike:

Thanks for the review!

Looking at the MR, it looks like it isn't exactly a revert ...

I had forgotten that I kept the test that @abhijith s (or @AbhijithS ... one is d.o, one is GitLab) wrote. As I said in #20, it is a very nice test.

The third commit is a clean revert (git revert ...).

See my Comment #26 for why the later commit was needed.

I think we should focus on getting this issue to RTBC.

The core committers talk to each other. If any one of them decides to review this issue, and they want to ask @nod_ about it, then they will.

I added #3550720: Refactor FieldUiJSTestTrait::fieldUIAddNewFieldJS() because it is brittle → with just the changes to the test trait. I think that reduces the scope of this issue, making it easier to review. It also gives us a MR to prove that this change does not break anything.

If the child issue gets fixed, then we can remove (4) from the Proposed resolution of this issue.

benjifisher → created an issue.

The pipeline passed.

I mentioned the discussion of .getAccessibleName() in Comments #47, #49 to #54 in the Proposed resolution section.

@kentr: FYI, this attention to the issue summary is one way I try to make it easier for the core committer to do the final review.

There is a test failure for core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5Test.php. It looks unrelated, and the test passes locally, so I am re-running the test.

I got a test failure for core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5Test.php on https://git.drupalcode.org/issue/drupal-3093378/-/pipelines/619981.

I am adding a little more detail to the Proposed resolution section of the issue summary.

@kentr:

Thanks for asking about my health. At this point, I am working three days a week and getting back to Drupal contribution. In a few months, I should be close to my "old normal".

If you are willing to take on the Reviewer role, then I will defer to you. I reverted your revert. Of course, the core committer will have the last word.

Referring to translated text:

Wouldn't the inner text content that is found and checked by .textEquals() also be affected by this?

Good point.

@cwilcox, thanks for weighing in.

Now the pipeline is failing ...

I will see if I can fix it. Maybe Tuesday.

I hope you will reach out to @larowlan and/or @quietone on Slack and discuss the possibility of large-scale changes to the tests.

Returns the computed WAI-ARIA label of an element.

If we translate the text, either using $this->t() in PHP code or Drupal.t() in JS, won't that be the translated text? Again, I was unable to try this out since I could not get the tests to run locally. I suppose I could push a temporary, do-not-merge commit and let GitLab CI do the test.

Thanks for reverting your commit.

I see myself more as a collaborator than a sole reviewer.

That's a bummer. Then we still need to find someone to review the issue.

Or maybe this is the time for you to take on the Reviewer role, which is very important. I think you are familiar with the code and test changes on this issue. Why not review it?

RTBC means Reviewed and Tested By the Community, not Ready To Be Committed. (It used to stand for that.) You are a member of the community. Your review represents your best effort to improve, then approve the issue. Do your best to ask the questions that a core committer would ask. Your best effort is good enough. If there is part of the MR that you are unsure of, be open. Tell them that you are not sure, and they will focus their attention in the right place. Some of my early reviews admitted that I had not looked closely at the tests, since I did not feel qualified. That's OK. Open Source Software is built on radical honesty and radical modesty.

When the core committer reviews the issue and asks for changes, you will learn something. Your next review will be closer to the goal of asking all the questions the committer would ask, which saves them the trouble of asking those questions.

@kentr:

I am sorry to be quiet for so long. I have been dealing with some serious medical issues. (If you want the details, then I can point you to a Slack thread.)

There are a couple of reasons I am nervous about using .getAccessibleName().

First of all, I am having trouble running the Nightwatch test locally. (See my Comment #46: "It took two tries, ...".) So I cannot tweak and evaluate as I would like.

Second, I searched the codebase and this seems like the first use of that test method. Maybe there is a reason it is not being used? Or maybe it should be used widely, in which case it might be a better idea to have a big issue for updating lots of tests instead of starting small with the test for this issue. In other words, I see a lot of value in keeping a single test self-consistent. Even if .getAccessibleName() is a better way to test, it might not be worth the inconsistency.

Third, I think .getAccessibleName() relies on the translated message. The usual practice in automated tests is not to use translated text unless you are testing translations. But see my first point above: I may be wrong about this.

There is also a meta-reason for not making this change. Until you made that test change, you preserved your role as a reviewer on this issue. I think we can restore that if we revert your commit. Then you have the authority to declare this issue RTBC. If we have both committed code to the MR, then we have to find someone else to review it.

I had an interesting discussion with @larowlan a while ago: https://drupal.slack.com/archives/C223PR743/p1753146459028029. I started by asking for some general advice on Cypress tests for the current project at my day job. Here is a bit of that thread:

A day or two ago, I was looking at some Nightwatch tests in the tooolbar module. They seem to use CSS selectors and nothing else.

-- @benjifisher

and

yeah so do a lot of our JavascriptFunctionalTests in core unfortunately

-- @larowlan

I was referring to the tests for this issue, of course.

Anyway, my point is that there is a lot of room for improvement in our current test suite. Maybe .getAccessibleName() is part of it, maybe not. But this is what I am thinking in my second point, where I suggest making broad changes to the tests instead of an isolated change to the test for this issue.

@kimpepper:

I see that you updated the MR. Should the issue status now be NR instead of NW?

@camhoward:

I am happy to discuss at length. It is unusual to revert an issue that has been committed and added to a full release, so we should be sure we know why we are doing it.

I have updated the issue summary, indicating the preferred approach. Are you willing to mark the issue RTBC? If you are not comfortable giving a code review, then maybe you can bring this issue up in DrupalEasy office hours (Comment #21) again and find someone who can do that.

Is there any advantage to reverting the change from #2219393 and then adding weights to the 32 YAML files?

My opinion is that reverting #2219393 is the correct fix for this issue. After that, if we want to add weights to some YAML files, then we can do that in separate issues: it seems out of scope for this one.

I had to re-read the comments to see that your "32 YAML files" refers to the second part of my Comment #22. I should have been clearer there: I was describing what we would have to do if we kept the alphabetical sort. My main point is that we should remove the alphabetical sort, and then we do not have to edit any YAML files.

@nicxvan, thanks for providing instructions on this novice issue!

I just want to add my thanks, too, since I am the one who created the issue.

You might get more help for questions like this on the #migration Slack channel. (If you do that, then please copy some of my reply to the thread. That will help avoid duplication of effort.)

I am afraid you are right, and adding another plugin is not going to work. The reason is that each process plugin has an input value (source) and configuration. The input value can come from an earlier step in the process pipeline. The configuration is static. Everything under attribute_map is configuration.

On the other hand, you keep using process: default_value. The other option (looking at the doc block of the plugin) is process: array_traverse. It has been almost 3 years since I looked at this issue, and I do not remember how it is supposed to work, but that looks like it is designed for your use case.

If you cannot get that to work, then you might need something kludgy. Like use a static value for data-entity-uuid, then have a separate step in the process pipeline to replace that with the correct value.

I think I created this issue, but I did not attend the meeting.

benjifisher → created an issue.

Only two people joined the Zoom meeting this week, so we decided to cancel the meeting.

I am adding credit for the contributors mentioned in Comment #70.

benjifisher → created an issue.

benjifisher → created an issue.

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