🇩🇪Germany @FeyP

Created the MR for both 1 and 2 from the proposed resolution. Suggested Composer commands tested on Drupal 10.3 and Composer 2.7.7. Tested that merging into existing scaffolding plugin config works as expected. Could use verification of my test results and maybe some proof-reading from a native speaker. Alternatively, the directory name is easily changed with the old text to just do part 1.

FeyP → created an issue.

Various steps in here refer to a remote named drupal-ISSUE_NUMBER. In some cases, these are probably examples for the Core example mentioned in the introduction, in which case this would be correct, but not always. The drupal part is actually the machine name of the project you're working on. So I think this should be updated in those places that do not directly refer to the example in the intro. For now, I didn't do it, because I'm not really sure if this documentation should only apply to core development. I ended up here from clicking the "about issue forks" link in a project issue and then clicking a link in the sidebar, so I think it should apply to all projects, not just core?

@smustgrave Sure. I pushed a new branch for 7.0.x to the fork and updated the code there to address my review. New MR for 7.0.x is MR 82. I also rebased MR 6.0.x and cherry-picked my commits there, pushed everything to MR 72, which is still for 6.0.x.

I tested the update hook on Drupal 10.3.0 with the 7.0.x MR and I think it works as intended now. Note that I didn't re-test the 6.0.x MR after my changes, although I don't see how this shouldn't work.

I added the string change for the description as a separate commit so you can easily revert it, if you don't agree with that one. Unfortunately, it looks like I don't have permission to resolve my own review comments on the MR 72, so they're still open.

Here is a config export after the update hook with a single BEF view, only change was this one, just as intended:

--- a/sync/views.view.better_exposed_filters_issue_3282228.yml
+++ b/sync/views.view.better_exposed_filters_issue_3282228.yml
@@ -124,6 +124,7 @@ display:
                   collapsible: false
                   collapsible_disable_automatic_open: false
                   is_secondary: false
+                  hide_label: false
                 select_all_none: false
                 select_all_none_nested: false
                 display_inline: true

FeyP → changed the visibility of the branch 3282228-add-option-to to active.

FeyP → changed the visibility of the branch 3282228-add-option-to to hidden.

Did a little bit of testing using the workaround patch in #4 and for now it looks like it works as promised (i.e. the bug is fixed with the mentioned limitations).

Let's set the issue status to needs work, since there is now a patch and there is also a more concrete proposed resolution in #4.

Also proactively tagging for IS update, since we're missing a proposed resolution. Would be good to update the IS to the latest state before setting to needs review again as well, otherwise the issue will surely fail the review for that.

Can anyone explain why this has been set to needs work?

It is needs work for comment #72.

Thanks for working on this issue. I have a use case for this functionality in one of my projects, so I did a quick review. Unfortunately, I noticed an issue with the update hook. I confirmed that the update hook probably doesn't work as intended on a patched BEF 6.0.5 on Drupal 10.2.6. Since we're touching the code again anyway, I also added some rather nitpicky stuff as well, which is totally optional.

Add deprecation of class properties.

It seems that the second link is what you would expect:

Yes, that was the page I expected. Thanks for pointing that out.

Still, on https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21File%21Fi... I would expect, that FileSystemInterface in the class hierarchy links to https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21File%21Fi... , which it doesn't. Those are different pages.

And I'd expect the box "Same name in this branch" on https://api.drupal.org/api/drupal/core%21core.services.yml/service/Drupa... , so that I can at least quickly switch to the page I'm looking for.

I think that this "services" detail pages might not be needed at all.

The services detail page is fine, if it links to the service class, but I agree it is probably not needed under the name of the interface the service class implements.

So this is useful: https://api.drupal.org/api/drupal/core%21core.services.yml/service/file_...
I think we can drop this one: https://api.drupal.org/api/drupal/core%21core.services.yml/service/Drupa...

When searching for FileSystemInterface in 11.x branch, the result doesn't look right. I get a page for the @file_system service in core.services.yml. The class providing this service implements the interface, but I'd expect a page listing all the constants and methods in the interface. I also don't see a "same name elsewhere in this branch" box (or whatever is the exact name of that box), so it can't be just the "wrong" page:

https://api.drupal.org/api/drupal/core%21core.services.yml/service/Drupa...

On the page for the FileSystem service, it says it implements FileSystemInterface, but there is no link, so it seems it is indeed missing:

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21File%21Fi...

And thanks for fixing #32 and for the interesting link to your presentation @fjgarlin .

Cool, you're using search api. If you ever write a case study on the API page, make sure to share the link with us, it might be interesting. And no rush on the reindex from my side, as long as the content is what I expect, I'm happy :)

Is it expected that I see `https://api.prod.cluster.drupalsystems.org/` in the URL on production instead of `https://api.drupal.org`?

Steps to reproduce:
- Go to api.drupal.org
- Search for `NodeInterface` in the sidebar.
- Click on the result/press enter.

Expected result:
- Redirect to `https://api.prod.cluster.drupalsystems.org/api/drupal/core%21modules%21n...`

Actual result:
- Redirect to `https://api.drupal.org/api/drupal/core%21modules%21node%21src%21NodeInte...`

Still doing more testing, but for now it looks great :)

Thanks for filing this issue.

We don't actually provide the titles of the links in the module. The module uses the links (including the link title) from Drupal Core's language switcher and makes them available for display as menu links.

If you want to change the language switch links, including the link titles, there are a bunch of contributed modules available that you should be able to install in addition to this module that allow for various alterations of the language switch links. Alternatively, you can use hook_language_switch_links_alter() in a custom module.

For your specific request, there is also an example in the FAQ on the project page that I think does exactly what you asked for by just changing a template file in your theme.

I just marked 🐛 FormHelper::processStates causes notices on type-less elements. Closed: duplicate as a duplicate of this issue.

Thanks for working on this issue. This looks very similar to 🐛 Notice: Undefined index: #type in Drupal\Core\Form\FormHelper::processStates() Needs work . The other issue doesn't have an MR already, but is older, has a patch with a kernel test, has way more followers and even more recent activity, so I'm going to close this one as a duplicate. It would be great, if you could continue working on this in the other issue. Feel free to reopen, if I missed something. Thanks again.

-      watchdog_exception('patchinfo', $e);
+      \Drupal\Component\Utility\DeprecationHelper::backwardsCompatibleCall(\Drupal::VERSION, '10.1.0', fn() => \Drupal\Core\Utility\Error::logException(\Drupal::logger('patchinfo'), $e), fn() => watchdog_exception('patchinfo', $e));

Noting that DeprecationHelper was introduced in 10.1, so using it to replace watchdog_exception() doesn't make sense in this case, because Error::logException() was also introduced in 10.1. Either way we'll have to bump the version constraint to 10.1 for the next version. So in this case we can just use the new API without using DeprecationHelper.

This is a known limitation of the update bot/rector, because it made the implementation less complicated. Likewise, it is a known limitation that info.yml files of sub-modules are not updated by the bot for now. The latter issue might change at some point in the future, but most likely not during this cycle.

We could already fix the watchdog_exception, but let's keep the version constraints as is for now until at least beta1 or even rc1, in case there will be new deprecations we need to deal with later.

fwiw +1 from me as well. The wording sounds good to me, but I'm not a native speaker so ymmv. A few observations:

Since this is for posting to issues, I'd use proper anchor tags for the links with a nice title. I think it looks better and also might give more information on what the material in question is about, e.g. "Video: Introduction to Contribution Best Practices for Organizations" instead of a random link to Youtube.

For my personal usage, I'd word "I will not be be providing credit for this issue." a little bit less explicitly, similar to "This issue may have been reported" further below. As a maintainer I might still choose to give credit to some of the people involved in the issue, but might still want to inform them using this nudge that it was an exception to the rule and please be more careful next time. I might apply this exception for obviously novice contributors or for people that provided constructive input on the issue after it had been already created for some time without any movement/reaction to my review from the original authors/contributors. My plan is to also use this for issues that have been created before the policy came into effect, so I'd like to post this nudge as a hint to it so that they are aware of it for the future, but might still give credit as an exception this time. I can surely adjust the wording as needed, but if such situations would be covered by default by slightly rewording that sentence, that would be ideal.

poker10 → credited FeyP → .

warning: "Warning: Give to trusted roles only; this permission has security
implications. It may lead to sensitive configuration values that are
specified in settings.php to become visible in the user interface, such
as API keys, tokens and other secrets."

Wording the message in this way may lead a user to the wrong conclusion, that leakage of such info is the only security implication, which is often not the case. Depending on the module, this might actually not even be the most severe implication, but rather one of the minor implications.

Also fwiw +1 for #238.

I should have updated the proposed resolution in the IS also, done that now.

Doh, forgot to set to Needs review...

Thanks @alexpott for the review.

W/r/t pointing out LanguageInterface::isLocked(), I closed the follow-up ✨ Add an API method to LanguageInterface to simply checking, if a language is a pseudo language Closed: works as designed issue as "Works as designed".

I agree that the changes you suggested are shorter, more logical and more readable, so I updated the code.

I took the opportunity to switch to an MR based workflow. Added the MR under review to the IS as requested elsewhere by @xjm.

Thanks for working on this. @alexpott pointed out over in #2904899-29: Invalid argument exception when changing language of node with menu link to und or zxx → that you can actually use $node->language()->isLocked(), if you need this, so I'm going to go ahead and close this as works as designed. Feel free to reopen, if you disagree.

I wasn't able to reproduce the test fail locally, so I used the opportunity to switch to an MR based on patch #44. Let's see what the pipeline says.

Thanks for reporting this issue. I tried to reproduce on Drupal 10.0.11 and unfortunately, I wasn't able to do so. I did the following:

1. I created a vocabulary named "categories".
2. I created an entity reference field "field_test" for the user entity, referencing taxonomy terms and restricted it to a vocabulary named "categories".
3. I created two published terms in the "Categories" taxonomy, "Term 1" and "Term 2".
4. I set up a role and gave it none of the permissions provided by Taxonomy or by Taxonomy Access Fix.
5. I created a user with that role and set field_test to reference "Term 1" for that user.
6. I added a hook (code below) to hide field_test for the edit operation and cleared all caches for the hook to become active.
7. I logged in as the new user and verified that I wasn't able to see field_test on the user edit form.
8. I saved the form.
9. I edited the hook to apply to field_xtest instead of field_test.
10. I looked at the form again both as the user I created and with user 1. For both users, the field was visible again on the form. With user 1 I was able to see the reference to "Term 1". For the user I created for testing, the field showed "- Restricted access - (1)".
11. I tried this again editing various other fields on the user edit form instead of clicking just save when the hook was active, no luck.

Here is the hook I used for testing:

use \Drupal\Core\Access\AccessResult;

function example_access_fix_entity_field_access($operation, \Drupal\Core\Field\FieldDefinitionInterface $field_definition, \Drupal\Core\Session\AccountInterface $account, \Drupal\Core\Field\FieldItemListInterface $items = NULL) {
  if ($field_definition
    ->getName() == 'field_test' && $operation == 'edit') {
    return AccessResult::forbidden();
  }
  return AccessResult::neutral();
}

I'd appreciate it, if you could provide more detailed info on how to reproduce the issue. Do you use a different version of core? I didn't yet have a chance to test on 9.5 or 10.1. Can you share your hook, maybe there is something different that triggers the issue? Anything else I might be missing? Any other contributed modules/custom code you use that might be candidates needed to trigger this?

Add new "Theme templates" section to the TOC.

Looks good to me. Thanks!

Reading this again, router => router was a bad example. Let's use an example where it's more obvious what the property name is and what is the service name. Also, the service previously assigned to the property must not necessarily be deprecated.

Add a section on how to deprecate injected service properties using \Drupal\Core\DependencyInjection\DeprecatedServicePropertyTrait, discussed with @lauriii in Slack: https://drupal.slack.com/archives/C1BMUQ9U6/p1698411358577049

Attached is a patch against 2.x-dev.

FeyP → created an issue.

@kunal.sachdev Thanks for creating the follow-ups. Can you please set the status of the follow-ups to Postponed and post a comment that they are postponed on this issue when you change the status? You can also add this issue as a parent issue or as a related issue. I don't think it makes sense for someone to work on these issues until this issue is fixed.

Also, I think it might be a good idea to list the follow-up issues somewhere in the issue summary, because this issue is pretty long and takes some time to read through, so a good issue summary that has all the relevant information is even more important for this issue to make it easy for core committers and other contributors new to the issue.

The issue summary also lists a remaining task (2.) that is not marked done yet. Can you please update the IS with the current approach in your implementation and/or with a reference to the follow-up you created, so that it is clear that the task is either done or out of scope?

W/r/t the last follow-up, a subsystem maintainer mentioned in #48 that the setting should be kept in field storage. Unless there is compelling evidence that the reasons given by them are maybe outdated in the meantime (they commented 5 years ago, so it might no longer be true due to some other changes in core), I think we should follow their advice and keep the setting in storage. If we think the setting should be in the field settings, I don't think we should have a follow-up, but I think we should do it in this issue. It doesn't make much sense to introduce a new setting now and then shift that from storage to field settings later. We could then use a field setting right from the start.

I could fix this only by deleting the cache tables in db. drush cr, did not solve it.

That's interesting. So it's not related to the code at all, but some kind of caching issue? I'll see if I can find something in that direction. Might be a few days until I can take a look though.

I think #87 is covered by 🐛 Claro: Form labels that are disabled have too low color contrast Needs work , which mentions border color of disabled elements in the IS, so we don't need a follow-up for that.

Form API states has various issues already filed against it, but doing a quick search, I didn't find any obvious candidates for #92. The tests only cover usage with TRUE, not with FALSE. I think we should file an issue for this, if it doesn't exist already, but I don't think we need to postpone on this. But I also think we shouldn't commit it like this, so let's go back to the previous behavior. We could then file another follow-up to use the current behavior using Form API states in this case to improve usability and post-pone that PP2 on this issue and the new follow-up for Form API states.

Tagging Needs Followup again for that.

I just closed 🐛 States JavaScript should use the once library, causes problems with Big Pipe Closed: duplicate as a duplicate of this issue. There is some information on how to reproduce with the contributed Webform module in the IS of that issue and MR with a similar but slightly different approach to fixing this.

Thanks for filing and working on this issue. This looks like a duplicate of 🐛 Form API #states property/states should use .once() to apply its rules (Can cause failures with BigPipe and possibly other situations) Needs work , so I'm going to go ahead and close this, since the other issue is older and has more followers. Feel free to reopen, if I missed something.

No response within a month. Closing works as designed for now. Feel free to reopen with more information.

Thanks for letting us know. Looking at the code in core I don't see any recent changes that we didn't address and I haven't had any issues myself on a bunch of systems.

Most likely this is either a core patch that you're using that modifies the route, another contributed module that alters this route or some custom code you use. Could you please check your systems for any candidates? If it's a contrib module, we might be able to accommodate with a patch.

volkswagenchick → credited FeyP → .

Thanks, good catch! I updated the existing kernel test for the processor to cover this issue. No interdiff, since it's trivial.

Attached is a patch against 4.x-dev.

FeyP → created an issue.

Attached is a patch against 8.x-1.x-dev.

FeyP → created an issue.

Attached is a patch against 2.0.x-dev/2.0.2.

Note that the patch adds filtering of the attribute value based on \Drupal\Core\Template\AttributeString::__toString(), only using Html::escape() directly since it was already used anyway, and a check, if the value is not empty. Technically, the first hunk of the patch should be enough to fix the bug.

FeyP → created an issue.

Thanks for filing this issue.

The module already provides an upgrade path for those new permissions, that were previously part of other old permissions. See the update hooks 9401 and 9402.

For the other permissions, these were not previously covered by any existing permissions, they are entirely new and allow different things. We can't just give arbitrarily new permissions to roles they didn't have before. It may seem "logical" for your project, but we can't assume that it will be the case for all projects that use the module. That's why the update hooks also ask you to review the newly assigned permissions, so that you can take the opportunity of the upgrade to make changes as necessary.

I'm not closing this as won't fix yet, because I wonder why you needed to add "view terms in VOCABULARY" to "view term names in VOCABULARY" (provided by 9401) and "view terms in VOCABULARY" to "select terms in VOCABULARY" (provided by 9402). Didn't those update hooks work for you? If so, can you tell us your module and core versions before and after the upgrade and also if you upgraded in multiple steps or a single step?

Sorry, bot, but you tested the wrong patch...

Thanks @lauriii for the clarification in #43, very helpful.

I replaced the data providers with foreach loops, this should address #41. Patch and interdiff without indentation changes provided for convenience of reviewers.

@sascha_meissner Thanks for working on this. The merge is blocked because there are new commits on 10.1.x and your branch has diverged, so a fast-forward merge is not possible. To fix this, you need to rebase the MR branch. You can see this when you open the MR page in GitLab:

> Merge blocked: fast-forward merge is not possible. To merge this request, first rebase locally.

See here for more info: https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... →

To get an early review or a second opinion for design decisions, you could ask in #ckeditor5 or #contribute in Drupal Slack.

Thanks for letting us know about this error. I think the problem is a missing use statement for EntityTypeManagerInterface. I have just updated the example with such a use statement. That way, you don't need to change anything else. Setting the type hint to the interface is generally recommended, since it allows to inject another class as long as it implements the same interface, so I wouldn't change that.

Add a use statement for EntityTypeManagerInterface to the example in step 3.

4.0.1 has been released. New issue for the next release: 🌱 Plan for Taxonomy Access Fix 4.0.2 release Active

FeyP → created an issue.

Merged. Thanks again!

Now had a chance to test the various upgrade paths manually and it seems to work as intended. RTBC.

Thanks once more for working on this! All feedback in the MR has been addressed and I can't find anything new :). I did another manual test on 11.x and everything works great.

The IS looks good and reflects the current state of the issue. The CR still reflects the current state of the MR. As I said previously, since both affected forms are marked as internal, I think we don't need a separate CR for the constructor changes, so I think we're good there.

We've got test coverage for all user permission forms in core, we got test coverage for the deprecation message in the constructors, we got test coverage for the system access content logic in UserPermissionsForm including some edge cases that might happen during filtering. Tests pass locally and on CI and removing some code in a few selected places, the tests fail as intended.

I think this is RTBC! 🎉

Reading the proposed resolution I wonder why this just being applied when saving a menu_link in the UI. Surely this can be done outside of the UI? And thus things will break? Am I not understanding something?

This is not an API issue. The issue is the way the Menu UI module uses the API to update a menu link. It tries to create a translation when it shouldn't. I tried to rephrase the proposed resolution so that this is hopefully more clear. Feel free to improve on that.

I think a followup is needed for #7, but as always, search first. Adding tag.

Filed ✨ Add an API method to LanguageInterface to simply checking, if a language is a pseudo language Closed: works as designed . If this needs to be postponed against a Drupal 11 meta or any tags or something, I'd appreciate your guidance.

Didn't do the review, so can't comment on that. Back to Needs review.

FeyP → created an issue.

Thanks, great work here! This access content logic is haunting us a bit... I've added a few questions and one suggestion inline.

And I forgot to answer your question w/r/t to the additional subscriber needed to expand test coverage to cover the edge cases (all node permissions filtered, system access content permission filtered): One thing that's often done in cases like this is to use the state service. You could get the permission names to (not) filter from state in the test module, then set the state with the permissions to (not) filter before getting the form. See it in action in the test coverage here for example, although in that case it's used the other way around. That way you only need one test module. As I said, I will not block on this, because I think hitting this edge case is fairly uncommon in practice, so a potential regression there not being caught by a test wouldn't be too bad, but maybe the info is still useful in case a core committer will.

Thanks for working on this, this is really almost there now :)

The new test coverage for the other permission forms looks good.

Unfortunately, I think we didn't get the fix for the "access content" permission logic quite right yet. I've made another inline comment about that. Let me know, if I missed something. So setting back to needs work for that.

Thanks. I now tested this manually. The contrib module doesn't seem to work with the current state of the MR (which is of course absolutely fine), so I wrote my own subscriber and in general it works great. Testing some edge cases, when I filtered away all permissions except one permission provided by user module I got this:

TypeError: array_keys(): Argument #1 ($array) must be of type array, null given in array_keys() (line 124 of core/modules/user/src/Form/UserPermissionsForm.php). when I filtered away all permissions provided by the node module.

I'm not sure how common it is that you'd get this in practice, but I think we should fix this. I don't think we need to add tests for that case, but I won't object, if you think it would be a good idea.

I made some slight adjustments to the existing CR to update it to the current state of the MR. Since both forms are marked as internal, I think we don't need to write a separate CR for the constructor changes, so I think we're good there.

W/r/t the test coverage we have that for the user permissions form and we have a deprecation test, which is great. I wonder if we should add the same test coverage we have for the user permissions form also for the other permission forms that extend this form, i.e. UserPermissionsRoleSpecificForm, UserPermissionsModuleSpecificForm and EntityPermissionsForm? What do you think? If you don't think it is required, I might RTBC without this, so feel free to convince me that it isn't needed :).

Setting to needs work to fix that edge case I mentioned above and potentially the additional test coverage. Then I think we should be good to go, so hopefully the final iteration from my end.

This is looking very good already!

I think the new event is a great idea. I also like how we now limit the API to its intended purpose by explicitly only allowing to filter permissions, which also makes for much nicer DX. Thanks for implementing this change!

I made another quick code review and left some comments on the MR. Nothing major, just one more question where you probably had your reasons and I just don't see why, and a few nits I noticed on the way. For now I'm leaving this at "Needs review" so that we can maybe get some more eyes at this.

Also made some minor changes to the IS.

Thanks @laurri for the review. Fair enough, I'll change this to foreach loops. Not sure yet if I have time left this weekend, might have to wait until next weekend.

Should we do the same in 🐛 Invalid argument exception when changing language of node with menu link to und or zxx Fixed ?

Thanks! Test results look good now and the code looks good as well. I didn't realize that the fix would be that easy, I thought we might have to move everything to post update hooks, which might have had other problems, or work on the role config directly or something...

I'd like to give this some manual testing before I commit this, just in case, will do that later. Then I can hopefully also take a look at the MR for core, unless @smustgrave beats me to it ;).

For now, I've added this as a known issue to the release notes. I'd also say this is a major issue.

Thanks for filing the issue.

The assumption was that you made the update of the module before upgrading core, a direct upgrade of both at the same time has never been tested.

Looking at the test results, it looks like this is failing, if you do the update in Drupal 9. I guess because then the dependencies are not fulfilled, because there is no such user update hook, yet?

Looks like we need to work on this some more.

For our sites, we have some reporting in place for patches and the results are aggregated in a dashboard. So this is possible, but I can tell you that it is very hard to aggregate the data even if you can force some rules (e.g. putting issue no. and comment id in the description in a certain format). Even in a small to medium sized agency like ours there are always some people who manage to "forget" the rules, which will mess up the results. Still, we find it useful to get a general idea about which patches we use. But for drupal.org, this would be a lot of work to implement properly, could be opt-in only (so potentially has the same problems as the module usage statistics that don't accurately reflect usage, especially on Drupal 9 and later where most sites use Composer), and is not of a lot of use when we switch to the MR based workflow exclusively at some point in the not so far future.

The "I use this MR" button sounds like an interesting idea in theory, but it is another thing that needs to be implemented on top of GitLab, people might forget to keep this updated, so the data is likely to be misleading, and it is prone to abuse, if you want to. E.g. when I start using a patch in our custom profile, I would have to set "I'm using this MR on 600 sites". If we want to support that, any user could easily misuse that to force the attention of the community at certain MRs they are interested in at a given time. I like to believe that our community would not do this, but the recent attempts at abusing the credit system by certain members of the community are a bad example and I think the business case of getting those issues fixed faster that you rely on is even more tempting than market place ranking. So I don't think this is something we should do.

Instead, I suggest we use what we already have. The number of people who follow an issue should be a good indicator about which patches are maybe more important than others. GitLab also has a way to up- and down-vote issues and they are using this to a certain degree during issue triage to prioritize resources. There is also a bot we could use to tag issues with a high number of followers or above a certain threshold of up-votes as more important than others. This might not be as accurate as a patch/MR usage dashboard, but it is most likely accurate enough for our use case and needs little to no time to implement, because most of what we need (including the data) is already there.

Maybe instead we should always add the token CSS

That would probably work for me. If you'd be willing to take it, I could write a patch for that. I'd just have to double check on a few systems first, maybe there actually is an example for a "bad" token library hidden somewhere in one of them...

if you are willing to start the "extra" module (which I feel could be very minimally maintained yet still useful) then it could go there.

As I said in the other issue, I'll seriously think about that, however I guess that would still require some API changes here, because we'd still need some way to access either the bubbleable metadata or the token libraries.

In TokenProcessorTrait we can put token libraries from the body

I agree that we don't need this for the subject. Body should be enough.

I was also unsure about how to best pass on the data, that's why I finally took the approach of adding the libraries directly in TokenTrait, because I didn't want to change the scope of the other methods and adding additional get/set methods to the interface just for this seemed like being a little bit too much. There is also still the issue of token replacement in mail addresses (that's eventually going to be another issue/patch ;). For now I have added that to AddressAdjusterBase, but ideally I'd need a way to access the token data (and token options) there to make this work nicely, which I can't... I didn't yet figure out how to best implement that as well.

Maybe having the possibility to add arbitrary metadata to the email object is a good idea. If we can't just always add the token libraries for some reason, then I'd suggest we look into that further.

Not really😃. My motivation for creating such a project is that maintaining this one is already enough work, so I'm interesting in keeping it as small as reasonably possible.

That's what I thought :). Then I'll have to think about whether I should create such a project or just add it as custom code.

Maybe we need to be able to choose per library??

It would probably be best if you were able to choose this per library, but then it also gets complicated. With the current implementation, you can just add the processor and if the CSS fits, which you can simply test, you're done. If not, you can choose to not use the processor and instead add to your own mail css library or just keep the plain output of the token without the library. I think in at least 80% of the cases, this yes/no choice per mailer policy should be enough. Everything else would either require more complicated configuration that might involve reading code (so I guess we don't want to do that) or you have some kind of hook or event that you need to re-write for every use case. If Symfony Mailer gained more traction or was merged into core, then I could imagine that this might be an additional key in the library definition or something like that. But for now, I don't think implementing a hook for that would make much sense. Then I could also straight-away implement a hook to inline that additional library for that type of mail policy.

Do you have an example where it would be bad to add the CSS library to a mail?

Not really, no. I just know that often you still need some hacks to support certain mail clients, especially if the markup or css rules get more complex. I think w/r/t email we're somehow still stuck in the 90s, so to say. In my opinion, developing an email template for a newsletter is still a lot like web development used to be before Mozilla Firebird/fox became a thing and the browser wars ended. My assumption was that CSS attached to tokens is most likely optimized for display in modern browsers and might therefore not necessarily be suitable for inclusion by default, so you probably wanted some kind of choice as a site builder.

Thanks for taking a look at this.

I wonder if this is a bit of a specialised case and perhaps it would be appropriate to be part of a separate project e.g. "Drupal Symfony Mailer Extra"?

Sure, we could also add it to such a separate project, if you think that would be better. I can also always add this to one of our custom modules, if it's not something for upstream entirely. For this new project, would you be willing to create and potentially maintain this? Then we can move the issue over. I could also do that, but honestly I'm not sure right now if I'll have enough time to properly maintain such a project in the long term.

I would propose not to have an "enabled" flag in many adjusters, and instead do this

Good idea. I'll look into this, might take a few days.

I guess replicating html2text in the test is not useful, so I put the test case of the base url in plain text on a separate line. That should hopefully fix this. Let's try again.

Alright, the base url of Drupal CI is so long that it adds more new lines than expected. Let me work on the tests some more and see, if I can fix this.

Attached is a patch against 1.x-dev.

FeyP → created an issue.

Doh, wanted to set this to Needs review, of course.

Attached is a patch against 1.x-dev.

FeyP → created an issue.

I agree that the approach proposed in #7 is very interesting. I might even volunteer to join such a group, if the community would trust me to do that. In the past, I didn't get involved in this d10readiness stuff for (supposedly) abandoned modules, because I usually have my hands full maintaining my own projects and I simply can't commit to maintaining such projects in the long term. However, just working on a release for the next major, maybe fix a few urgent issues, preferably for projects that I already use, and all that without joining as an official maintainer of the project, would work well for me. It would also be transparent to the community that people like e.g. @heddn don't have plans to maintain the module in the long term, but that this is a one-off gig.

I also know of a few projects where the maintainers just don't have the time "right now", but haven't actually abandoned the project and would appreciate such temporary help to get out a release. A few weeks ago I talked to someone from another agency, who maintained a module that was supposedly abandoned, and they really appreciated the offer by the d10readiness initiative to help get the release out, because they just can't work it into their schedule at the moment. If such a group existed, the DA might even send out a mail to all maintainers offering porting help and there could be a way for maintainers to explicitly request that someone from the group work on their module and take care of a release, which might make some things easier.

W/r/t #11.5, there are different ways to deal with that. If there are good alternatives that are well-maintained, one option might be to not release a compatible version, but provide an upgrade path to one of the alternatives. The difficulty with such an approach would be that existing maintainers might object to that, if the project is not really abandoned. Another obvious option is to come up with some kind of criteria on which modules to work on, which is already part of the proposal, because it is required for the governance part of the process. Finally, there is room for innovation even when such a group would create a compatible release, given that such a module would most likely be less than minimally maintained. So there is still an incentive for innovation, it might just not happen as quickly.

You could use the contributed term_condition module, if you need such a condition. I don't think this needs to be re-implemented by menu_position. I suggest to close this as works as designed.

I've completed the survey. Compared to other recent surveys by other initiatives, this one was relatively easy to complete. I thought about forwarding this to a few clients, but I guess if you only know the editors side of the menu you might struggle with sorting this, because there might be a lot of options you've never seen before.

Adding 🌱 Categorize contributions that could be considered 'gaming the credit system' and propose solutions (policy, automation, etc) Active as a related issue.

hestenet → credited FeyP → .

Thanks for working on the MR. This looks very good already! I have added a few inline comments, mostly not that important, but I think I have identified one case where the update hook still needs work.

I also wonder, if we should override ConditionPluginBase::calculateDependencies() and add additional dependencies on the term uuids, in case a term will be deleted? But that might be out of scope for this issue.

I tested the upgrade hook with menu_position upgrading on Drupal 10 from 2.0.1 to the MR and it seems to work well for that case.

Unfortunately I don't use eca or rules. At least for eca the tests have some examples, e.g. https://git.drupalcode.org/project/eca/-/blob/1.2.x/tests/modules/entity... So I think you are correct in that the existing update hook probably won't work for eca.

Additionally, it looks tome like 2.0.2 switched to using term UUID instead of term ID (#2695141), but provided no update hook for existing installations.

Yes, for some reason only changes to existing files from the original patch were committed, but not new files, which included a config schema and an update hook, which are now missing. To me, it doesn't look intentional, but like some kind of oversight. Note that the update hook only updates block configuration, but condition plugins can be used in other places as well. Some popular contributed modules using condition plugins coming to mind are rules, eca or menu_position. So even if the upgrade path had been committed, it might not have been sufficient for your use case. I think it might be hard to find and update all relevant configuration that might be using condition plugins, so this looks like an API break. In hindsight it might have been better to release this change in a new major instead of in a patch release. Also, I didn't yet look at the original patch closely to see if the array vs string issue would still need fixing even with the additional files committed.

quietone, smustgrave, andy-blum, dww, cilefen and I discussed an older policy issue about self-RTBCing core issues in Slack. During the discussion, it was suggested that we might use a bot to help with issue triage. Such a bot could make suggestions when a new issue is created, updated or stalling. This could range from information on how to get more eyes on an issue, e.g. an auto comment for issues tagged 'Needs usability review' that states that there is a regular usability review meeting, to potentially other triage tasks based on issue metadata that could easily be automated. cilefen mentioned GitLab's https://gitlab.com/gitlab-org/ruby/gems/gitlab-triage bot, which could be used to implement this. It could also be used for one-off issue updates or to post introductions to new features as suggested in the IS. More might be possible through plugins. Examples for the policies in use at gitlab.com can be found here: https://gitlab.com/gitlab-org/quality/triage-ops/-/tree/master/policies Just for reference, quietone posted a good summary of the discussion in #2706483-45: [policy, no patch] Policy to help less interested Patch move forward. → .

While Windows support wont be as bad as #29, I would note, I believe technically any Windows bug would likely be no more than a priority Normal because it has "no significant impact" and is by community consensuses is "not important". That absolutely applies to Windows + Apache in the current scenario of dropping all production Windows support. That can indeed lead to incompatibilities given time as it receives less and less targeted support. I can't speak to most core dev's, but I can say I know I wouldn't focus on Windows issues if its not a supported production environment platform.

While this is probably true, I think this is already the current state of affairs. It was already noted previously in this issue, that there is no test coverage for Windows and not a lot of Windows users are involved in the issue queue, so an issue affecting only Windows is not likely to get a lot of attention, which is one of the reasons why it was proposed to drop support in the first place. If however a serious compatibility issue would arise and enough Windows users worked on a fix or were available to test fixes and RTBC the issue, a fix would probably still be committed.

Say I discover a vulnerability, for sake of argument something like say SA-CORE-2023-005. I report it using ONLY Windows examples because I only developed and tested on a Windows machine. Ultimately *nix is also vulnerable, but all the examples I provide are Windows and I make no acknowledgment of *nix because either I didn't test it at all or I couldn't think of how to use the same vulnerability in *nix.

This is probably a scenario that we would have to trust the security team will catch during triage and I'm willing to do that. I think looking at how a Windows issue might affect *nix is most likely a lot easier than looking at how a *nix issue might affect Windows given the current composition of the team. I'm certain the team can and will handle that.

This is why I tend to generally lean to fix any security vulnerabilities in private, and to fix them quickly

I'm not going to comment on the private vs public issue, I think you know my opinion on that one and in the end that is a related but different discussion that is already going on elsewhere in the queue. But fixing issues more quickly is also one of the reasons that were mentioned here for making this change. From following the comments here I get the impression that in the past some fixes for *nix were delayed because there was not sufficient resources to look into how an issue affected Windows or to validate a proposed fix on Windows. Given the current involvement of Windows users in the issue queue (just based on my personal impressions and what has been said here previously, not based on hard numbers of course) this isn't likely to change anytime soon, especially if we're also looking for security experts.

Isn't it then ultimately better and more honest to our users and community to acknowledge that we just don't have the resources to support Windows at the moment instead of keeping the status quo?

Thanks Angie also from me for all you've done over the years. It was always a pleasure to work with you! You'll always be a part of our community through the impact you've had and the memories we share, but I hope that maybe, just maybe some day in the future you'll be able to play a more active role in our community again. Until then, I wish you all the best and may the drop be always with you :)

any issues reported publicly.

I think what this means is, that any security issues that only affect Windows will be reported (and potentially dealt with) in the public issue queue. So if you are on Windows and you want to work on the (hypothetical) future incompatibility/get the issue to RTBC, that's fine, go ahead. As long as you're only running Windows in development, you should be fine. It doesn't mean that we don't care about an issue a user might have on Windows + Apache for example, it just means that if it's a security issue, the issue will be filed in the public queue, where it would otherwise have been dealt with in the private issue queue.

Note that escaping the equal sign is not enough to prevent CSV injection. OWASP has a list of characters to escape, @larowlan already posted that one. This list is also used by Smyfony in its serialization component: https://symfony.com/blog/cve-2021-41270-prevent-csv-injection-via-formulas So the patch most likely needs to be updated to include the additional characters. Setting to "Needs work" for that.

There was still one more reference to the closed alpha, so I took the liberty to make some more adjustments to the IS.

Another random test failure, retest was green, see 🐛 [random test failure] InstallerExistingConfig[SyncDirectory]MultilingualTest::testConfigSync Fixed , back to RTBC per #39.

Random test failure, see 🐛 [random test failure] InstallerExistingConfig[SyncDirectory]MultilingualTest::testConfigSync Fixed , retest was green. Back to RTBC per #20.