🇺🇸United States @benjifisher

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.

Thanks for the feedback.

I think we should not implement the two convenience methods. There are several reasons:

The has...() convenience method calls hasDestinationProperty() and (if that returns FALSE) hasEmptyDestinationProperty(). If the code next calls the remove... convenience method, as in your snippet, it calls the same has... method(s). That seems sloppy.

The method names are long, and they do not adequately describe what the methods do. Anyone reading the code will have to look up what the method does. It is convenient to write, but not to read.

Adding extra methods means we have to add and maintain test coverage for them.

In simple cases with content entities, you can get the same effect by adding overwrite_properties to the configuration of the destination plugin. (See Drupal\migrate\Plugin\migrate\destination\EntityContentBase.) I think this covers the most common use cases. I do not want to add convenience methods to the API if they are only going to be used by occasional projects that have especially complex business logic. Those projects can add convenience functions (not methods on the Row class) themselves.

Thanks for suggesting this addition to the Migrate API. I think this is more of a feature request than a bug report, so I am updating the category.

I am trying to understand the use case for this addition. Is it mostly for migrations that update existing entities? If a migration is creating a new entity, then I think it does not matter whether the Row object has a null property or is just missing that property.

I also wonder whether we need all four of the new methods. Are hasDestinationOrEmptyProperty() and removeDestinationOrEmptyProperty() really useful? Again, what is the use case?

For both of those questions, it would help to see a code snippet of how you are using (or plan to use) the new methods.

benjifisher → created an issue.

@daffie:

The two snippets in the issue summary use $query->join('inner', ...) and $query->join('left', ...). I think that is a little odd, but it is all right since they are not explicitly described as equivalent.

But the first before/after example in the change record (CR) has the same examples. There, I think it is a problem: an inner JOIN is different from a left JOIN, isn't it?

I will not move the issue back to NW for that, but I will add an issue tag.

I have an even smaller suggestion about another example in the CR:

$condition = \Drupal::database()
  ->condition('AND')
  ->compare('d.entity_id', 'b.entity_id')
  ->compare('d.entity_type', 'b.entity_type')
  ->compare('d.field_name', 'b.field_name');

As I read it, the coding standards → encourage breaking before each method if each method belongs to the same object. Here, condition() is a method on the Connection object, and the three compare() methods belong to the Condition object. So I would rewrite that snippet (saving one line) as

$condition = \Drupal::database()->condition('AND')
  ->compare('d.entity_id', 'b.entity_id')
  ->compare('d.entity_type', 'b.entity_type')
  ->compare('d.field_name', 'b.field_name');

The CI tests passed, but just because the "Validatable config" test is allowed to fail.

TL;DR: I think it is worth looking at that test, but I think it is OK for it to fail for this issue.

I am not familiar with that test, but it seems to compare the results of

vendor/bin/drush config:inspect --statistics

on the target branch (11.x) and on the feature branch.

On the target branch, the installs drush and the config_inspector module. Then it installs the Standard profile and enables all core modules (except sdc, which is obsolete) and config_inspector. On the feature branch, the test first applies database updates.

The test fails if any of the statistics are lower on the feature branch than they are on the target branch.

I went through similar steps, but instead of vendor/bin/drush config:inspect --statistics, I exported configuration. The only difference between the two branches is that the feature branch includes core.base_field_override.node.article.promote.yml. It looks as though this config entity is not fully validatable: the schema is in core/config/schema/core.data_types.schema.yml, and it is not marked as FullyValidatable: ~. So it makes sense that the statistics go down and the test fails.

I am copying the participant list from the private meeting notes.

benjifisher → created an issue.

benjifisher → created an issue.

benjifisher → created an issue.

Add a brief description of Cross Site Request Forgery, with links to more complete explanations.

I agree it is a good idea to have an extra level of authentication before making changes to the codebase, but I think that "password authentication" is too restrictive.

Some sites use single sign-on (SSO) or other methods of authentication, so requiring a password would not be appropriate. On the other hand, it may be that sites using SSO also have dev, stage, and prod versions of the site, so they could use the proposed dev/prod toggle.

It is out of scope for this issue, but I think that Drupal core should provide an authentication API. That would make it possible, in this issue, to let the site owner choose the method of authentication. It would also simplify the tfa (Two Factor Authentication) module.

Usability review

We discussed this issue at 📌 Drupal Usability Meeting 2025-04-25 Active . That issue will have a link to a recording of the meeting.

For the record, the attendees at the usability meeting were @benjifisher, @rkoller, @shaal, and @worldlinemine.

We like the idea of breaking up the search page, as this issue does. But instead of making two separate pages (with two entries in the Administration menu) we would like to have two tabs (with one entry in the Administration menu). This is similar to the approach used for Languages (/admin/config/regional/language), where the first tab has a list of available languages and the second tab (Detection and selection) has settings.

Once the list of search pages is on a separate page (or tab) we also think it would be helpful to add some help text to that page. Again, the Languages page (List tab) is a good model: it explains where the ordering and default are used.

We also spent some time discussing the "Indexing progress" section, and whether it belongs on the main tab (list of search pages) or the settings tab. In the end, we agreed with the decision made here, to keep it with the settings.

We also think that a useful addition would be to have a button that immediately runs cron, just for the Search module. That button should go on the main tab, where the indexing progress of each page is listed. But it should not really be "immediate": it should open a confirmation form, just like the "Re-index site" button. The confirmation form should check whether cron is currently running; if so, then offer options to check again or to cancel. Otherwise, give some information (something like "314 of 1000 items are indexed; continue to add 100 items to the index"), a link to the settings page, and a confirmation button.

But that is out of scope for this issue.

In summary:

1. Recommendation: use two tabs instead of two separate pages.
2. Suggestion: add some help text to the main tab (list of search pages).
3. Follow-up issue: add an "Index now" button to the main tab.

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

benjifisher → created an issue.

Updated from discussion on the Migrate video call today. benjifisher, heddn, mikelutz and quietone were present. 📌 [meeting] Migrate Meeting 2025-04-24 2100Z Active

Two of the blocking issues have been Fixed, and the third is Closed (won't fix).

Updated from discussion on the Migrate video call today. benjifisher, heddn, mikelutz and quietone were present. 📌 [meeting] Migrate Meeting 2025-04-24 2100Z Active

Two of the blocking issues have been Fixed, and the third is Closed (won't fix).

Updated from discussion on the Migrate video call today. benjifisher, heddn, mikelutz and quietone were present. 📌 [meeting] Migrate Meeting 2025-04-24 2100Z Active

We discussed this issue at 📌 [meeting] Migrate Meeting 2025-04-24 2100Z Active and decided to close it as Won't Fix.

The minimum_version and requirements_met attributes are all or mainly used by the migrate_drupal module, but they are Mostly Harmless, so we decided not to spend any effort on removing them.

I am adding a rough transcript and adding related issues. See also the child issues of 📌 Tasks to deprecate Migrate Drupal Active .

Rewrite and expand the documentation. Include

• CSRF protection in the Form API
• The _csrf_confirm_form_route option
• The _csrf_request_header_token requirement
• The system.csrftoken route

Add "Structure of routes" as related content. It is a sibling of the parent guide.

Here are a few suggestions for the agenda:

1. Converting source plugins from annotations to attributes
2. Deprecating the migrate_drupal and migrate_drupal_ui modules
3. Editing migrations in the admin UI

We converted source plugins to attributes in 📌 Convert MigrateSource plugin discovery to attributes Active . In a follow-up issue, 📌 Move source_module and destination_module from Migrate to Migrate Drupal Needs work , we removed source_module from the attribute class and reverted all the source plugins that extend DrupalSqlBase to use annotations (since they need to declare source_module).

Are there any other follow-up issues that need attention? Maybe 📌 Remove mimimum_version and requirements_met from MigrateSource attribute class Active ? If we want to do that, should we push to get it in before Drupal 11.2.0 is released? (That will be the first release that includes the attribute class, so BC is much less problematic if we change the attribute class before then.)

Other recently fixed issues: we moved two source plugins from migrate_drupal to migrate in 📌 Move content_entity source plugin to migrate module Active and 📌 Move the d8_config source plugin to the migrate module Active . Now we can have same-site migrations, using those source plugins, without enabling the migrate_drupal module. I now feel much better about deprecating the migrate_drupal module. Should we go ahead and do it? Let's review the plan issue, 📌 Tasks to deprecate Migrate Drupal Active .

When I added the Import CSV Recipe → , I realized that we are missing one important step for "ambitious site builders": they have no good way to edit (or even inspect) migration plugins. I think the Modeler API → module (currently in development) will help fill that need. It pulls out the code from the ECA → module that interacts with the JS-based editor. When it is ready, it will provide a tool that we can use to build a great editing experience for migrations (defined as config entities thanks to the migrate_plus module). First step: get the modeler_api module to an alpha release. (I think it is close.)

benjifisher → created an issue.