🇺🇸United States @benjifisher

Fix up formatting.

Avoid contractions ("Here's"), shorten the text (Less is more!), and avoid a silly decision (is it "a SQL" or "an SQL"?).

heine → credited benjifisher → .

xjm → credited benjifisher → .

xjm → credited benjifisher → .

poker10 → credited benjifisher → .

poker10 → credited benjifisher → .

We accidentally made two issues for this meeting. I am closing this one as a duplicate of 🌱 Core security triage 2024-10-31 Active .

> Migrate existing security issues. Do we need closed issues, or lose those forever?

I think we need the closed issues. As a general rule, I hate to throw away information.

If an issue was considered a valid security issue, then fixed, then some of the important information is in the public SA. Even then, a lot of details about the possible exploit are only in the issue (now closed).

If an issue was considered not to be a valid security issue, then at best we have a comment in a public "hardening" issue. The discussion and the details are only in the issue (now closed). We often look for old issues similar to newly created ones: "in previous issues very similar to this one, we decided to fix them in public because ...".

I resolved several threads. I left open the ones where I made a suggestion and it has not been implemented. I still think these are improvements.

Unfortunately, one fix seems to have been lost, so back to NW. It is pretty small: just a missing |null in an @var comment.

johnpicozzi → credited benjifisher → .

johnpicozzi → credited benjifisher → .

Remember to check #1473760: Use data-* to check modules dependencies before submit at the 2024-11-22 meeting and close it if there are no further comments.

I checked the issue credits against the private notes that the security team keeps. LGTM

benjifisher → created an issue.

benjifisher → created an issue.

amateescu → credited benjifisher → .

I see two test failures on the latest pipeline (yesterday) for https://git.drupalcode.org/project/drupal/-/merge_requests/6111/pipeline... (for 📌 Implement Entity::fields() for migration destinations Needs work ):

1. Drupal\Tests\image\Functional\ImageStylesPathAndUrlTest::testImageStyleUrlAndPathPrivate
2. Drupal\Tests\responsive_image\FunctionalJavascript\ResponsiveImageFieldUiTest::testResponsiveImageFormatterUi

These two tests seem unrelated to the issue, and both tests passed when I re-ran them.

I forgot to review the failing tests:

1. Drupal\Tests\image\Functional\ImageStylesPathAndUrlTest::testImageStyleUrlAndPathPrivate
2. Drupal\Tests\responsive_image\FunctionalJavascript\ResponsiveImageFieldUiTest::testResponsiveImageFormatterUi

I added a note to 🌱 [meta] Known intermittent, random, and environment-specific test failures Active and re-ran the tests. They both passed.

@baltowen:

Thanks for fixing that.

Next time around, please add additional commits instead of making your changes as part of a rebase. I had to compare to the tag previous/2630732-11-x-migration-destinations-entity-fields/2024-11-05 in order to make sure that there were no accidental changes added during the rebase.

The merge conflicts were in core/modules/migrate/tests/src/Unit/destination/EntityRevisionTest.php. I reviewed that carefully, and the changes there look good to me. Back to RTBC.

I am replacing the screenshot in the issue summary and rearranging a bit to match the usual template. (Screenshots go under "User interface changes".) I am replacing "more appropriate text" with "empty text" in the Proposed resolution section.

My screenshot looks a lot like the one in 🐛 User logout is vulnerable to CSRF Fixed .

Previous comments and the Remaining tasks suggest adding an automated test, but I do not think that is needed.

Code looks good, the confirm form is what I show in the screenshot, and the confirm form works. RTBC.

Well, I tried to re-run the pipeline. I am not sure it is working.

If we do want automated tests, then we could test that the default text, "This action cannot be undone", does not appear on the page.

It is usually a bad idea to suppress phpcs warnings, but I think it is the right thing to do in this case.

There is a failure in Drupal\Tests\layout_builder\Functional\LayoutBuilderBlocksTest::testBlockPlaceholder. That test is already listed on 🌱 [meta] Known intermittent, random, and environment-specific test failures Active , so I am re-running the tests. (I do not see a way to re-run a single test suite, so I am re-running the entire pipeline.)

I have not tested, but the code change looks correct. I am setting the status to NR for testing and further review.

Comment #9 asked for automated tests, but I am not sure that is needed.

The issue summary needs an update (proposed resolution and screenshots), so I am adding the tag for that.

@baltowen:

Thanks for fixing that. There is still one line that needs to be fixed: see my comment on the MR. I wish I had caught that sooner.

There is also one unrelated test failure: Drupal\Tests\block\Functional\BlockCacheTest::testCachePerPage. Again, let's not worry about that until we fix the problem we know about.

@baltowen:

Thanks for working on this issue. Unfortunately, core/modules/migrate/tests/src/Unit/destination/EntityRevisionTest.php fails after the changes you made. Back to NW.

There are also some unrelated tests that fail. For now, do not worry about those.

This issue adds the $entityTypeBundle class variable to the test class. After your rebase, that variable is handled correctly: call reveal() in the setUp() method, not in the test methods. But your rebase also changes the way $storage is handled. That is not in scope for this issue, and Comment #4 on 📌 Drupal\Tests\migrate\Unit\destination\EntityRevisionTest uses weird mocking pattern Active explains why that one variable should not follow that pattern.

If you undo the changes to $storage in the test class, then that test should pass. Then we can worry about any unrelated test failures.

From Comment #20:

Just wanted to confirm this one because as noted above, this text wasn’t there when the change was committed. Added a screenshot to the IS.

I tested by checking out Commit 62c4d2d2fa6 (the commit for 🐛 User logout is vulnerable to CSRF Fixed on the 10.3.x branch). I installed Drupal with the Standard profile and visited /user/logout/confirm. The text was there: "This action cannot be undone." It is true that the screenshot on #144538 does not show that text, but the text was there when the issue was committed.

I was curious, so I investigated.

The problem is this commit on the MR for #144538: b6134f16, which removed this function (and added return-type declarations to 5 other functions):

  public function getDescription() {
    return '';
  }

The same day, the author of the commit made this comment #144538-186: User logout is vulnerable to CSRF → :

I pushed a change to add some typehint returns. If we aren’t adding a description I removed that function.

Actually found an issue that return ’’ doesn’t match the docs of the inherited docs.

That is a mistake. The text, "This action cannot be undone", comes from the base class. Removing that function means we are not overriding the base class.

Later that day (Comment #188), the author of the commit (and Comment #186) marked the issue RTBC. It is not good practice to review your own work.

The second line of Comment #186 makes the same observation as Comment #8 on this issue. Because the form class implements an interface, getDescription() has to return a TranslatableMarkup object, not a string. But it can return the object equivalent to an empty string:

  public function getDescription() {
    return $this->t('');
  }

Usability review

We discussed this issue at 📌 Drupal Usability Meeting 2024-11-15 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, and @simohell.

Our recommendation is to remove the text, "This action cannot be undone". Do not replace it with anything. The purpose of the form is clear without this additional text.

In other words, we strongly agree with Comment #3. And Comment #20.

I will add a separate comment with some technical notes.

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

Remember to check ✨ Use data-* to check modules dependencies before submit Active at the 2024-11-22 meeting and close it if there are no further comments.

Usability review

We discussed this issue at 📌 Drupal Usability Meeting 2024-11-15 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, and @simohell.

In #35, @nod_ raised the question of whether we should remove the confirmation step. The Usability team thinks we should keep that confirmation.

The cost of the confirmation step is small. Enabling modules is not something you do every day on a site, like editing content. There are some site owners who manage many sites, but they normally enable modules with drush.

There is sometimes a large benefit to the confirmation step. If a site owner wants to evaluate a new module, they may not be aware of its dependencies. (Before we used composer, this was less true. Site owners had to download all the dependencies individually.) What if one of the dependencies is a module the site owner does not want to install? Perhaps it duplicates or interferes with another module, already installed, or perhaps it has only an alpha release.

It can be difficult to uninstall modules. Occasionally, a bug in the module makes it impossible to uninstall from the UI. If there is a long dependency chain, then uninstalling requires several visits to the uninstall page.

We plan to close this issue (won't fix) in two weeks (2024-11-22) unless there is further discussion by then.

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

The link goes to Section 3.6 of the user guide, not 3.7.

benjifisher → created an issue.

No good deed goes unpunished: our follow-up issue 📌 Drupal\Tests\migrate\Unit\destination\EntityRevisionTest uses weird mocking pattern Active was recently Fixed, and it creates a conflict with this issue.

Resolving the conflict is a little more than what I am comfortable doing while acting as a reviewer on this issue. (I already did that once: see Comment #144. The changes now are a little more complicated.)

I rewrote the git history, combining all the commits that change core/modules/migrate/tests/src/Unit/destination/EntityRevisionTest.php, and force-pushed. You can confirm that I only changed history by comparing with the tag for the previous HEAD of the feature branch:

$ git diff previous/2630732-11-x-migration-destinations-entity-fields/2024-11-05 && echo done
done

That should make it easier to rebase on the current 11.x. I think that is considered a Novice task, so I am adding that tag again.

@andypost:

I filed https://github.com/Nevay/spi/pull/5 to prevent updating core's vendor hardening

I think what you mean is that we will not need to update the config for drupal/core-vendor-hardening if that PR is merged. Either way, nothing prevents us from updating it.

I updated the issue description (Remaining tasks) based on Comments #19 and #20.

I checked that tbachert/spi is the only new package that includes tests/ when installed with composer, so that is the only one that needs an addition to drupal/core-vendor-hardening.

I checked that there are no other packages that depend on google/protobuf, so the major-version upgrade should not have any surprises:

$ ddev composer why google/protobuf
open-telemetry/gen-otlp-protobuf 1.2.1 requires google/protobuf (^3.22 || ^4.0)

That is with the feature branch (or running composer update -W open-telemetry/* myself). With 11.x, I get this, which explains the major-version upgrade:

$ ddev composer why google/protobuf
open-telemetry/gen-otlp-protobuf 1.1.0 requires google/protobuf (^3.3.0)

I added some more detail to the Proposed resolution in the issue summary.

+1 for RTBC

I notice that the MR for 10.4.x sorts the entries under allow-plugins. +1 for that.

In that section, both MRs add "tbachert/spi": false. That seems like the right thing to do based on the comment #3478895-19: Document new Composer plugin tbachert/spi required by core-dev → :

Right now, opentelemetry should work just fine with that plugin disabled (it's currently only used by a new declarative configuration feature that's still being actively developed).

I ran composer update open-telemetry/* myself (using the latest 11.x) and got the same changes to the two composer.json files. +1

I also tried composer update -W open-telemetry/*. That also updated google/protobuf. I guess our practice is to update all PHP dependencies at once when preparing a new minor release, so there is no need to update that now.

+1 for RTBC

@liam morland:

When d.o provides package metadata for Composer, it inspects the .info.yml. Adding an entry for Drupal core to composer.json is redundant, and it would mean that you have to update the compatibility information in two places going forward.

I cannot review this issue and mark it RTBC, since I worked on the MR. But the current MR is pretty simple:

1. Remove requirements for php and drupal/core from composer.json.
2. Update core_version_requirement to ^10.2 || ^11. That is, drop compatibility unsupported versions of Drupal core.
3. Fix an unrelated typo in the .info.yml file.

What we really need is for the active maintainers to agree on what to do. Personally, I agree with @dcam in #13.

Usability review

We discussed this issue at 📌 Drupal Usability Meeting 2024-11-08 Active . That issue will have a link to a recording of the meeting.

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

We agreed that, of all the child issues of 🌱 [Plan] Improve field creation experience Active , either this issue or 📌 [PP-1] Move label and machine name fields to the field config edit form Active would be a good next step.

We discussed the advantages of using modals in this process. Modals speed up the process by reducing the number of full page loads. They make keyboard navigation easier, since tabbing only goes to elements within the modal. They also make it easier to keep track of the current context and to return to the start of the process if the user decides to cancel.

In the past, there have been some drawbacks to using modals, mostly related to accessibility. The current situation is much better, thanks to improvements in recent versions of jQuery.

Ideally, Drupal would provide a framework that makes it easy to implement multi-step forms in modals. (Probably there is already an issue for this.) If that is not done before this issue, then perhaps the work here can serve as a model for how to implement that.

Usability review

We discussed this issue at 📌 Drupal Usability Meeting 2024-11-08 Active . That issue will have a link to a recording of the meeting.

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

We agreed that, of all the child issues of 🌱 [Plan] Improve field creation experience Active , either this issue or 📌 Move the first two steps of field creation into a modal. Needs work would be a good next step.

Experience shows that site builders frequently miss the required Label field (and the related, hidden by default, machine name field) on the current form. Moving it to the next step in the process would be an advantage. A further step (for some future issue) would be to go to the next step after choosing a field type in one step (select an option) rather than two steps (select an option, then submit the form).

benjifisher → created an issue.

Thanks for pointing out that problem. I edited Comment #15.

I compared the issue credits to the notes in the security team's private Google doc. LGTM.

benjifisher → created an issue.

larowlan → credited benjifisher → .

I cherry-picked and squashed the commits from the original MR onto the 10.4.x branch. Is there an easier way to test on 10.4.x?

An unrelated FunctionalJavascript test failed: Drupal\Tests\ckeditor5\FunctionalJavascript\ImageUrlTest.

Some unrelated Functional tests failed:

• Drupal\Tests\block\Functional\BlockTest
• Drupal\Tests\block\Functional\BlockCacheTest

I re-ran the failing tests, and now they are all green.

benjifisher → created an issue.

I commented on 🐛 Inconsistent use of language regarding plugins Active .

Usability review

We discussed this issue at 📌 Drupal Usability Meeting 2024-10-25 Active . That issue has a link to a recording of the meeting.

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

We agree that the language here is inconsistent, but we think the right way to fix this is to remove references to "plugins", not add more. The word "plugin" has too many meanings (in Drupal, CKEditor, and WordPress, for example) and someone configuring the toolbar is more likely to think of "features", "buttons", or "toolbar items".

We also think we should use something more generic, like "toolbar items", instead of "buttons", since the toolbar also includes drop-down lists and separators.

We suggest making the following changes to the interface text:

1. Change the label "Toolbar buttons" to "Toolbar items".
2. Change the text "Move a button into the Active toolbar to enable it, or into the list of Available buttons to disable it." to "Move a button into the Active toolbar to enable it, or into the list of Available items to disable it. "
3. Change the text "The toolbar buttons that don't fit ..." to "The toolbar items that don't fit ...".
4. Change the header "CKEditor 5 plugin settings" to "Settings".
5. Change the help text for "Manually editable HTML tags" (under the "Source editing" vertical tab) to
   

   A list of HTML tags that can be used while editing source. It is only necessary to add tags that are not already supported by other enabled toolbar configuration. For example, if "Bold" is enabled, it is not necessary to add the tag. If there is no "Definition list" item, then it may be necessary to add
   

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.

The motivation for this issue was to find a better solution for 🐛 Add missing category to Drupal\layout_builder\Plugin\Layout\BlankLayout and let modules and themes alter the list of layouts Fixed . It is tempting to do that as part of this issue: in Drupal\Core\Layout\LayoutPluginManager::getLayoutOptions(), replace the line

    $filtered_definitions = $this->getFilteredDefinitions($this->getType());

with

    $filtered_definitions = $this->getFilteredDefinitions();

Unfortunately, #3392572 was fixed in Drupal 10.3.0, and it is possible that some module has already implemented hook_plugin_filter_layout__layout_alter() instead of hook_plugin_filter_layout_alter(). So I do not think we can make that change without first deprecating that hook.

I added a draft change record (CR), so I am removing that issue tag: https://www.drupal.org/node/3481873 → .

@annmarysruthy, thanks for continuing to work on this issue.

My first comment on the MR was this:

Since this function implements a method from an interface, we should also update the interface: ...

I thought about this some more while writing the CR, and I think I was wrong. From Signature compatibility rules in the PHP docs:

A signature is compatible if it respects the variance rules, makes a mandatory parameter optional, adds only optional new parameters and doesn't restrict but only relaxes the visibility.

So we are allowed to make the parameter optional in the trait without changing the interface. On the other hand, if we make the parameter optional in the interface, then we will break any contrib module that has a custom implementation of getFilteredDefinitions(). I cannot think of how we can warn module maintainers with a deprecation notice before causing their code to fail.

I am leaving the status at NW.

@lavanyatalwar:

Thanks for fixing that. It may feel like a small thing, but helping to finish an issue that is 99% done has as much impact as completing an unstarted issue all by yourself.

Some unrelated tests were failing. That seems to happen a lot recently. Maybe I should add a note on 🌱 [meta] Known intermittent, random, and environment-specific test failures Active , but I was lazy and just re-ran the failing tests. They all pass, now.

Back to RTBC, The CI job and I agree that @lavanyatalwar correctly added the return type reported by PHPStan. I am updating the "Remaining tasks" in the issue summary and removing the Novice tag.

benjifisher → created an issue.

I checked the issue credits against the notes in the security team's private notes. LGTM

benjifisher → created an issue.

xjm → credited benjifisher → .

I think this issue needs some discussion before we start implementing anything. In the issue description (IS), I wrote,

Here are some options to consider:
...

I am a newbie to drupal community ...

Thanks for looking for ways to contribute. This issue might not be the best place to start. Have you looked for issues with the "Novice" tag? For example, I added that tag to #2630732 last week in the hope that someone would jump on it.

I did not include it in the issue summary, but one option is to restrict the 'View any unpublished content' permission less broad. I think this option would be too disruptive. Existing users on existing sites would suddenly not be able to see unpublished entities that they currently can see.

If we add a new permission, or one per entity type, then we have to consider how to implement that permission in views. We should also consider how it affects typical workflows with the content_moderation module.

I am adding some work-arounds to the issue summary.

I wonder if EntityPublishedInterface, or the base class \Drupal\Core\Entity\EditorialContentEntityBase, should add a helper method to be used in access() methods of entity classes or in implementations of hook_entity_access(). The helper method could handle metadata for caching, which is important for access control.

I am adding issue credit for the users who commented on the private issue.

Unfortunately, the term "content" has several meanings in Drupal. Sometimes, it means "node entity". Sometimes it means "content entity". As I explained in the issue summary, it means something in between those two in the context of the 'View any unpublished content' permission.

There are at least two places in the content_moderation module that incorrectly suggest that the permission applies only to nodes. The route provider has these lines:

        // If the entity type is a node, unpublished content will be visible
        // if the user has the "view any unpublished content" permission.
        ->setRequirement('_entity_access', "{$entity_type_id}.view")

and content_moderation.views_execution.inc has this function:

function content_moderation_views_query_substitutions(ViewExecutable $view) {
  $account = \Drupal::currentUser();
  return [
    '***VIEW_ANY_UNPUBLISHED_NODES***' => intval($account->hasPermission('view any unpublished content')),
  ];
}

benjifisher → created an issue.

I must have been doing something wrong, because the one-line change seemed to be working yesterday. On further testing, it does not.

The current MR adds an empty file (empty.csv) to the codebase. If the path is set to the specific string '/dev/null', then the constructor replaces that (in $this->configuration) with the path to the empty file and also sets header_offset. The result is that the source plugin returns zero rows, without errors, warnings, nor exceptions.

Other benefits.

With the feature branch, I can roll back a migration:

$ ddev drush mr import_csv_terms
 [notice] Rolled back 7 items - done with 'import_csv_terms'
$ ddev drush ms import_csv_terms
 ------------------------- ------------------ -------- ------- ---------- ------------- --------------- --------------- 
  Group                     Migration ID       Status   Total   Imported   Unprocessed   Message Count   Last Imported  
 ------------------------- ------------------ -------- ------- ---------- ------------- --------------- --------------- 
  Import CSV (import_csv)   import_csv_terms   Idle     0       0          0             0                              
 ------------------------- ------------------ -------- ------- ---------- ------------- --------------- --------------- 

With the 8.x-3.x branch, I cannot roll back a migration unless it specifies an actual path:

$ ddev drush mr import_csv_terms

In UnavailableFeature.php line 40:
                                    
  stream does not support seeking.  
                                    

Failed to run drush mr import_csv_terms: exit status 1

With the feature branch, when I check the status of my migrations, I get this:

$ ddev drush ms
 ------------------------- ------------------ -------- ------- ---------- ------------- --------------- --------------------- 
  Group                     Migration ID       Status   Total   Imported   Unprocessed   Message Count   Last Imported        
 ------------------------- ------------------ -------- ------- ---------- ------------- --------------- --------------------- 
  Import CSV (import_csv)   import_csv_terms   Idle     0       7          -7            0               2024-10-14 13:40:21  
  Import CSV (import_csv)   import_csv_users   Idle     0       3          -3            0               2024-10-14 13:40:21  
  Import CSV (import_csv)   import_csv_nodes   Idle     0       3          -3            0               2024-10-14 13:40:21  
 ------------------------- ------------------ -------- ------- ---------- ------------- --------------- ---------------------

With the 8.x-3.x branch, I get errors (but the same information):

$ ddev drush ms
 [error]  Could not retrieve source count from import_csv_terms: stream does not support seeking. 
 [error]  Could not retrieve source count from import_csv_users: stream does not support seeking. 
 [error]  Could not retrieve source count from import_csv_nodes: stream does not support seeking. 
 ------------------------- ------------------ -------- ------- ---------- ------------- --------------- --------------------- 
  Group                     Migration ID       Status   Total   Imported   Unprocessed   Message Count   Last Imported        
 ------------------------- ------------------ -------- ------- ---------- ------------- --------------- --------------------- 
  Import CSV (import_csv)   import_csv_terms   Idle     N/A     7          N/A           0               2024-10-14 13:40:21  
  Import CSV (import_csv)   import_csv_users   Idle     N/A     3          N/A           0               2024-10-14 13:40:21  
  Import CSV (import_csv)   import_csv_nodes   Idle     N/A     3          N/A           0               2024-10-14 13:40:21  
 ------------------------- ------------------ -------- ------- ---------- ------------- --------------- ---------------------

I think this can be done with ✨ Allow /dev/null as the source path Active . I am adding that as a related issue.

MR 14 is a simple, 1-line change.

I am adding ✨ Allow a callback function for the Source path parameter Active as a related issue. I do not see an easy way to solve this problem with the proposed resolution from that issue, but maybe there is a way.

benjifisher → created an issue.

I agree with #17. I think #16 is mis-reading the table.

In #3384600-68: Don't hide permissions local tasks on bundles when no permissions are defined → , @catch suggests that the last three rows of the table are withing the margin of error. Taking the numbers at face value, the winner is the core patch (#3384600, fourth row), not the patch on this issue (fifth row).

The only reason we might want to apply the fix in this issue is because the core issue will not be in a release until at least 10.4.0.

One of the phpstan tests reports,

150 Fetching deprecated class constant EXISTS_REPLACE of interface
Drupal\Core\File\FileSystemInterface:
in drupal:10.3.0 and is removed from drupal:12.0.0. Use
\Drupal\Core\File\FileExists::Replace instead.

The change record FileSystemInterface replace behaviour constants deprecated and replaced with an enum → suggests some ways to support new and old versions, but I suggest not making any changes as part of this issue.

Would you like me to open a Postponed issue to change this when the minimum version goes to 10.3, or just wait for the automated updates for Drupal 12 compatibility?

But that is so easy!

I have been looking at this module along with migrate_plus and migrate_source_csv. Both of those declare compatibility with Drupal core >= 9.1, and I did not notice that this one declared ^10.1 || ^11.

Back to NR.

The change record (CR) does not mention how to support multiple versions of Drupal. If we agree that the approach I took is good enough, then I will add something to the CR.

I tested manually with Drupal 11. It is already past my bedtime, so I am not going to test with an older version of Drupal. Maybe GitLab CI will do that for me.

Does this bug fix need regression testing?