🇺🇸United States @benjifisher

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.

Oops, I updated the wrong issue. Back to NW.

benjifisher → created an issue.

The title of this issue refers to "Field sql storage", and the proposed resolution mentions Drupal\Core\Field\FieldItemInterface::schema(). But 📌 Allow field types to control how properties are mapped to and from storage Needs work refers to \Drupal\Core\Entity\Sql\SqlContentEntityStorage, and I think that is where the actual calls to unserialize() are.

Then I guess you mean the usage in the dblog module:

$ grep -ri @unserialize core
core/modules/dblog/src/Controller/DbLogController.php:      $variables = @unserialize($row->variables);
core/lib/Drupal/Core/Config/DatabaseStorage.php:    $data = @unserialize($raw);

The @ just tells PHP to ignore errors from serialize(): see Error Control Operators in the PHP docs. There are many other calls to serialize() in Drupal core.

In the long run, we should switch to using json_encode() and json_decode() (or the equivalent methods in Drupal\Component\Serialization\Json) whenever possible instead of serialize() and unserialize(). In the short run, it is less disruptive to specify allowed_classes.

Thanks for the updates.

I think we can compromise and keep the has... method. After all, it is handy to have it in the tests.

In addition to the unit test, I would like to add a functional test. Basically, it comes down to the question I asked in Comment #12: I would like an example in a test (as well as an answer here on the issue) to "What is the use case?"

I think the functional test needs a test module with two migrations. Each migration can use the embedded_data source plugin. The first migration can populate two fields, and the second migration can set both fields to empty. (To be honest, I am not sure what it takes to make that happen. Maybe just foo: null in the embedded data and field_foo: foo in the process section?) Then the module should implement an event listener, like the snippet in Comment #15 and remove just one of the two empty destinations.

Then the test can run the first migration to create an entity, then run the second migration to update the entity. After all that, assert that one of the two fields is empty and the other is not.

If I got any of that wrong, then I mis-understood the answer in Comments #13, #15. In that case, it proves that we need the functional test.

This issue is scoped to the Config system, and I think there is only one:

$ grep -ri unserialize core/lib/Drupal/Core/Config
core/lib/Drupal/Core/Config/DatabaseStorage.php:   *   The unserialize() call will trigger E_NOTICE if the string cannot
core/lib/Drupal/Core/Config/DatabaseStorage.php:   *   be unserialized.
core/lib/Drupal/Core/Config/DatabaseStorage.php:    $data = @unserialize($raw);

Did I miss something or are you thinking of something outside the Config system?

I am testing this issue while at MidCamp, so I added the issue tag for that. @cosmicdreams is here, too.

Testing instructions

I started with the current version of Drupal 11.x (core, not recommended-project) and I am using ddev. Make changes as needed if you are using a different setup.

Install modules and Drush

Install from source, so that we can use git to pull different versions.

ddev composer require --prefer-source drupal/bpmn_io:^3@dev drupal/modeler_api:^1@dev drupal/ai_agents:^1.1@dev
ddev composer require drush/drush

Check out specific branches

I copied and pasted the instructions from this issue. (Expand the "Show commands" section.)

cd modules/contrib/ai_agents
git remote add ai_agents-3524652 git@git.drupal.org:issue/ai_agents-3524652.git
git fetch ai_agents-3524652
git checkout -b '3524652-show-case-modeler' --track ai_agents-3524652/'3524652-show-case-modeler'

Then I switched to the dev branch of bpmn_io:

cd ../bpmn_io
git switch 3.0.x
cd ../../..

Install Drupal and enable modules

ddev drush si --yes
ddev drush en ai_agents_modeler_api --yes

Generate a one-time login link

ddev drush uli

Open the link in a browser.

Edit an AI agent

1. Go to Administration > Configuration > AI > AI Agents Settings (/admin/config/ai/agents).
2. Find an agent with Type: config, such as field_agent_triage.
3. Follow the Edit link.

Alternatively, from /admin/config/ai/agents, use the "Add AI Agent" action link (/admin/structure/ai_agent/add).

Testing results

When I enabled the ai_agents module, I got three warnings like this:

Schema errors for ai_agents.ai_agent.content_type_agent_triage …

I also see those warnings on /admin/config/ai/agents after clearing caches.

My guess is that these warnings are not related to this issue. They might be there because I am using the latest 11.x version of Drupal core, not any release.

Initially, I enabled the ai_agents module without the ai_agents_modeler_api submodule. That leads to a WSOD when I try to edit an agent. I think we need to disable the Edit link unless the appropriate modules are enabled. If this issue is intended as a proof of concept, then that can be done as a follow-up issue, but for now I am setting the status to NW.

I have worked with Emma on a few of the weekly Usability meetings and in BoF sessions at DrupalCon Vienna and DrupalCon Atlanta. Emma is thoughtful, knowledgeable, and easy to work with. We are lucky that Emma volunteered to join the Starshot Advisory Council, and I think that Emma is perfect for the newly created UX Manager role for Drupal core.

I think it is all right to allow stClass, but tests are passing with ['allowed_classes' => FALSE], so let's at least consider that.

Does this issue need a change record? If there are any contrib or custom modules that extend the DatabaseStorage class, and they need to allow other classes in unserialize(), then the good news is that they can easily override the decode() method, which is now

  public function decode($raw) {
    $data = @unserialize($raw, ['allowed_classes' => FALSE]);
    return is_array($data) ? $data : FALSE;
  }

benjifisher → created an issue.

I added , ['allowed_classes' => FALSE] everywhere. Let's see what that does to the automated tests.

benjifisher → created an issue.

@berdir:

See Comments #13 and #20. Also #35: @dcam tried to do it for the base class and ran into trouble.

@dcam, do you want to try again? If it ends up being a lot harder, then (for the reasons in #35) I suggest that we commit the current MR and open a follow-up issue.

I am giving credit to the participants listed in the Security Team's private notes.

benjifisher → created an issue.

benjifisher → created an issue.

i guess @benjifisher might be familiar with it from discussions in the security team?

I can neither confirm nor deny.

benjifisher → created an issue.

On second thought, I do not think we need to add hasEmptyDestinationProperty().

First reason: it is safe to call removeEmptyDestinationProperty() without first testing hasEmptyDestinationProperty().

Second reason: if code does need to test, it can just use in_array($field_name, $row->getEmptyDestinationProperties()).

Source and destination properties need the has... methods because they use the NestedArray methods, but $emptyDestinationProperties is just a flat list of strings.