🇫🇷France @prudloff

There is a D11-compatible release: https://www.drupal.org/project/system_stream_wrapper/releases/2.1.0 →

I added a test.

Thanks for pointing the SEO issue, I did not think of that.
If this is a breaking change, I'm less convinced we should do it.
How was this handled when the token was initially added?

I tried using zxcvbn to estimate the strength.
It gives a score of 2, which means "provides some protection from unthrottled online attacks" and estimates the time to guess with 10 requests per second to 4 months.

This is a good enough protection in most scenarios, however it is often possible to send a lot more requests per second if the server does not have rate limiting.
Just expanding the token to 10 characters would require 31 years at 10 requests per second.

prudloff → created an issue.

It seems GitLab automatically converts the MR to a draft if a commit starts with fixup!, it's a bit annoying.

If we decide to keep this feature, then I think we should fix ✨ Allow class types in item properties in SDC Active . The current behavior is confusing.

Yes I think we have to deprecate both the routes and the controller.

Currently the MR does this in user module:

• The routes are deprecated (this does not trigger a deprecation when generating a link to these routes because they are not aliases).
• The controller is deprecated (for static analysis tools).
• Instantiating the controller triggers a runtime deprecation.

And in rest module:

• The controller is duplicated (same code but without the deprecations).
• Added new routes (with different path) that use this new controller.

If we want to keep the same paths, I think we could do something like this in the rest module instead:

• If the rest module is installed, alter the user routes to use the new non-deprecated controller.
• Add new non-deprecated route names that are aliases of user routes (for now).

How would that work? Can two routes have the same path?
Or do you think we should remove the routes from the user module instead of deprecating them?

Tests are now green.

Thanks, I did something similar. But by looking at the code from 📌 Support route aliasing (Symfony 5.4) and allow deprecating the route name Needs work it seems the deprecated key does nothing if the route is not an alias (it is still useful if someone is looking at the YAML but it will not trigger a runtime deprecation).
But having this info in the YAML + a CR might be enough here?

Is there a use case for using the REST login route without having the REST module installed?

I think you could also be using this route with jsonapi.

Some tests RestLoginHttpTest are now failing, I need to investigate why.

I'm not sure how to deprecate the route.
We only support deprecations on route aliases but when can't make user.login.http an alias of the new route because users without the rest module will not have the new route.

I rebased the MR.

deprecated requires alias

prudloff → made their first commit to this issue’s fork.

It seems installing both seo_pager and pager_metadata does what you need without any extra configuration.

While I see the value of this change, I am wondering if this would not be out of the scope of this module.
Currently the module only adds metadata but does not alter the pager at all.

prudloff → made their first commit to this issue’s fork.

I fixed the failing test.

prudloff → created an issue.

prudloff → created an issue.

I converted the patch to a MR and made it work also in VideoPlayerListFormatter.

prudloff → made their first commit to this issue’s fork.

This can indeed be used to find a list of Drupal websites: https://publicwww.com/websites/%22X-Generator%3A+Drupal%22/

Our users confirm that the current behavior is confusing, especially if a term has a lot of synonyms.
Maybe we could add an option that allows to choose between the two behabiors?

I just saw this comment: https://git.drupalcode.org/project/glossify/-/blob/b29c09bbfd7cae8b82a73...
So it seems to be deliberate that every synonym is replaced once.

prudloff → created an issue.

prudloff → created an issue.

I don't think it's a bug, node_list is invalidated when any node is updated, so node:1234 is redundant here.

When a node is updated, Drupal invalidates both the node:xxx and node_list tags.

There is a contrib module that does this: https://www.drupal.org/project/disable_user_1_edit →
(Paranoia also does this but it has more features.)

I think it is a valid concern that you currently can't give someone the permission to edit users without preventing them from gaining more privileges by editing the admin user password.
We often want our clients to be able to create users but we don't want them to gain admin privileges.

There is also a cspell warning but it will be fixed by 📌 drupalci.yml can be removed Active .

prudloff → created an issue.

I added a test that fails without the patch and passes with it.

prudloff → changed the visibility of the branch 3.x to hidden.

prudloff → created an issue.

Instead of adding arbitrary tags to the list, wouldn't it make more sense to add a way to alter the list?
This way modules could allow tag needed for their specific use case.

(WordPress has a hook that does this: https://developer.wordpress.org/reference/hooks/wp_kses_allowed_html/)

We use getDisplayName() in #markup at least here:

• NodeForm::form()
• UserController::userTitle()
• UserNameFormatter::viewElements()

The patch works correctly in UserFormatter but causes double-escaping in other places (in the toolbar or in username.html.twig).
According to the doc of getDisplayName(), it should not have to take care of escaping the HTML, but this is not always true (as we've seen in UserFormatter).
I opened #3550121: The doc for AccountInterface::getDisplayName() says that string will be escaped but it's not always true → to clarify that.

prudloff → created an issue.

Just noticed this does not only affect the translation, it is also true for the original config.

I tried to reproduce this by adding a translation of the anonymous user name that contains HTML.

The summary mention these places where the HTML would not be escaped:
Seven: not in core anymore
In the toolbar: the HTML is correctly escaped.
In UserNameFormatter with link_to_entity enabled: the HTML is escaped (and truncated)
In UserNameFormatter with link_to_entity disabled: the HTML is not escaped (but is sanitized by the XSS filter). This could be confusing and should be improved.

I merged the changes from 🐛 Fatal error when using a Twig defined variable as Attribute value Active .

Maybe we could have two clock services implementing ClockInterface? One that returns current time and one that returns request time.

prudloff → created an issue.

The proposed solution could also help for checking a #access value.
I have seen contrib modules do things like this:

// This won't work correctly with an AccessResultForbidden object.
if (!isset($element['#access']) || $element['#access']) {
  // Assume the element can be accessed.
}

Providing an access way to check the access value would encourage them to check it correctly.

prudloff → created an issue.

Note that the current behavior is inconsistent between FormBuilder::isElementAccessible() and Renderer::doRender().
If #access on a form input is a string:
Renderer::doRender() will display the input (because it hides it only if #access === FALSE).
FormBuilder::isElementAccessible() will not process the input (because it processes only if #access === TRUE).

This would be fixed by only allowing valid values.

I think we need a test showing that the form can be altered in the same way in different scenarios (register, edit own user, edit other user).

I fixed the D10 test.

@bbrala The MR already has some tests that check arity keys in JsonApiFunctionalTest:
https://git.drupalcode.org/project/drupal/-/blob/868a9723aea3a084a2f8714...
https://git.drupalcode.org/project/drupal/-/blob/868a9723aea3a084a2f8714...

(The code you are pointing to is about /jsonapi/node/article/[uuid]/relationships/[field_name] but this MR is about/jsonapi/node/article/[uuid]/[field_name] (relationships already have arity keys that are added only when the relationship contains the same entity twice).

I added some arity keys to write tests (POST and PATCH) to make sure it does not break anything to send this key.

I also did some tests and indeed it seems it is not possible to make MySQL use an index for this blob column.

Maybe an alternative could be to sort by type/name from the locales_location table?
This would still be a predictable order and an index on varchar columns should work.

I merged the latest 11.x.

I added a minimal slot that shows the problem to one of the test components.

I added a condition for <nolink>, <none> and <button> because these routes have a special handling in LinkWidget::getUserEnteredStringAsUri().

I created a new MR that targets 11.x.

Do we have any doc about profiling this kind of change?

I fixed the code style issue reported by the bot.

I agree it would make more sense to display this on the text format edit form instead of only when saving it.

prudloff → changed the visibility of the branch 11.x to hidden.

Indeed noopener is not as useful as before because it is implied by target="_blank" and noreferrer.

However I think noreferrer is still useful and important for privacy.

I added a deprecation to see how many tests this would break.
This seems to be used in some components in core.

I opened a followup that would make the warning more useful: ✨ Don't overridde default security warning when providing custom warning on a permission Active

prudloff → created an issue.

prudloff → created an issue.

prudloff → created an issue.

Added explanation about "restrict access" and "warning" keys

Turns out this is already supported since #1987418: user.module - Convert theme_ functions to Twig → : https://git.drupalcode.org/project/drupal/-/blob/5cbac86546d697154f1eee4...

It's just never used in core and not documented on https://www.drupal.org/docs/roles-and-permissions → .
It is documented in the docblock of PermissionHandlerInterface::getPermissions() though.

I opened a MR to see how many tests this would break.

I think we should include in the MR the additional tests that were in the patch until #24.

prudloff → created an issue.

The token was added by #1922814: Denial-of-service vulnerabiility in Image module → but I could not find an explanation for the length. I suppose it was to keep the URL more readable?
I removed the length limit and it does not seem to break anything.

I added a deprecation that we can later turn into an exception.

prudloff → changed the visibility of the branch 3526250-notify-the-developer to hidden.

I merged the latest 11.x.

So this seems to be toughing a number of different modules and components. Should this be broken up?

Most of the changes are because these parts of the code add an attribute with a dangerous protocol and don't expect it to be filtered, so now they need to wrap the attribute in a Markup object to explicitly prevent it from being filtered.
I guess theses changes could be committed separately but they won't do anything or make a lot of sense on their own.

You can probably use this module to do this: https://www.drupal.org/project/r4032login →

I rebased the MR.

I merged the latest 11.x.

Sure we want to change to change ViewAjaxTest to use Nodebase? Doesn't that make node a hard dependency in view

I think the reason was that the test entities don't have a whole lot of cache parameters so using a view with nodes makes it easier to test that it has the correct cache tags like node_list (see #21).
But maybe that's a bad idea.

I rebased the MR.

prudloff → created an issue.

greggles → credited prudloff → .

greggles → credited prudloff → .

greggles → credited prudloff → .

greggles → credited prudloff → .

prudloff → made their first commit to this issue’s fork.