🇺🇸United States @benjifisher

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

I copied the list of participants from the security team's private notes.

benjifisher → created an issue.

benjifisher → created an issue.

I am adding this page to the menu.

We skipped the meeting on May 22, so I am re-purposjng this issue for today's meeting.

I missed that meeting. (I was traveling to Florida DrupalCamp.)

I copied the list of attendees from the Security Team's private notes. Any other member of the team can check the list.

Yes, the filter_id process plugin is needed only when upgrading from Drupal 6 or 7. It should be deprecated as part of this issue.

I am updating the title to clarify that we are not planning to deprecate all process plugins. My title is a little awkward, so feel free to improve on it.

benjifisher → created an issue.

benjifisher → created an issue.

Update links so that visitors who follow them will not be redirected.

I did not update any links that use the canonical path /node/{nid} except to remove the protocol and server.

Looks great! Thanks for putting up with all my requests for tweaks.

I sometimes have to remind myself that the T in RTBC stands for "tested".

For testing purposes, I installed Drupal with the Umami demo profile (and sample content). I then hacked the log process plugin, hard-coding a field name:

  public function transform($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
    // Hack for testing purposes.
    if (empty($row->getSourceProperty('field_preparation_time'))) {
      $row->removeEmptyDestinationProperty('field_preparation_time');
    }
    // ...
  }

I created a test module with the following migration:

id: empty_destination_test
migration_tags:
  - Test
label: 'Test removeEmptyDestinationProperty()'
source:
  plugin: embedded_data
  ids:
    nid:
      type: integer
  data_rows:
    - nid: 9
      field_preparation_time: null
      field_number_of_servings: null
process:
  field_preparation_time: field_preparation_time
  field_number_of_servings: field_number_of_servings
  nid:
    - plugin: log
      source: nid
destination:
  plugin: entity:node
  default_bundle: recipe
migration_dependencies: {}

It calls log after the two numeric fields have been processed: the order matters!

As expected, when I executed the migration (drush mim empty_destination_test), field_number_of_servings was emptied out and field_preparation_time was untouched:

MariaDB [db]> SELECT entity_id, field_preparation_time_value
    -> FROM node__field_preparation_time
    -> WHERE entity_id = 9;
+-----------+------------------------------+
| entity_id | field_preparation_time_value |
+-----------+------------------------------+
|         9 |                           10 |
+-----------+------------------------------+
1 row in set (0.001 sec)

MariaDB [db]> SELECT entity_id, field_number_of_servings_value
    -> FROM node__field_number_of_servings
    -> WHERE entity_id = 9;
Empty set (0.001 sec)

I feel a little guilty about pushing changes without any testing at all, but that is all I have time to do right now. At least, we will see whether GitLab CI finds any problems with my changes.

Thanks for adding the test, and +1 for making it a Kernel test. I have just a few suggestions on the MR.

@rkoller:

Thanks for reminding me of this issue on Slack. I agree, we can consider this issue Fixed.

I commented on 💬 Better help text for Date and Time Field Active .

@arpitr:

From the screenshot in the issue summary, I assume you are thinking about the text on the page (now in a modal window) when adding a new field. (For example, start at /admin/structure/types/manage/page/fields, "Create a new field", then choose "Date and time".) We have been discussing that text, along with the text for the other field-type groups, on 📌 [PP1] Refine field descriptions Active . I am adding that as a related issue and giving you credit over there. Please join the discussion there.

... intend here is to bring this to discussion to broader group in community.

If you are interested in this sort of issue, then please join the #ux Slack channel. If you are available on Fridays at 1400 UTC, then we are always happy to have additional points of view for the weekly Usability meeting. The Zoom link is posted in the Slack channel about 10 minutes before each meeting.

@arunsahijpal:

You are also welcome to join the Slack channel and the weekly meeting.

Looking at the MR you created, I think it creates a Help page for the Datetime module. That is related to @aritr's suggestion, but different from it. If you want to keep this issue open, then please update the issue summary, removing the existing screenshot and adding one showing the page you have added. If you are not interested in making those updates, then I think we can close this issue as a duplicate of 📌 [PP1] Refine field descriptions Active .

I am giving credit for the related issue 💬 Better help text for Date and Time Field Active .

I am adding credit for the users who contributed to 📌 [PP-1] Improve field description texts for fields provided by core Active , which was closed as a duplicate of this issue.

Since we closed this issue as a duplicate of 📌 [PP1] Refine field descriptions Active , I am adding that as a related issue.

I am adding credit for @tstoeckler from 📌 Adapt field type labels after switch to multi-step field creation Active . (Thanks to @rkoller for linking that issue to this one.)

Thanks for the review.

Since migration is marked to be removed in D12 and issues are being postponed might be good to get this in now.

The plan is to remove the migrate_drupal and migrate_drupal_ui modules. The migrate module is going to stay in Drupal core (maybe not forever). MR !12166 includes changes for all three.

I have added a change record. The automated tests pass. @rkoller and I have updated the issue summary. I think this issue is ready for review.