Lille
Account created on 11 April 2019, over 6 years ago
#

Merge Requests

More

Recent comments

🇫🇷France prudloff Lille

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?

🇫🇷France prudloff Lille

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.

🇫🇷France prudloff Lille

prudloff created an issue.

🇫🇷France prudloff Lille

prudloff created an issue.

🇫🇷France prudloff Lille

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.

🇫🇷France prudloff Lille

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.

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

prudloff created an issue.

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

prudloff created an issue.

🇫🇷France prudloff Lille

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/)

🇫🇷France prudloff Lille

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

  • NodeForm::form()
  • UserController::userTitle()
  • UserNameFormatter::viewElements()
🇫🇷France prudloff Lille

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.

🇫🇷France prudloff Lille

prudloff created an issue.

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

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.

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

prudloff created an issue.

🇫🇷France prudloff Lille

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.

🇫🇷France prudloff Lille

prudloff created an issue.

🇫🇷France prudloff Lille

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.

🇫🇷France prudloff Lille

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).

🇫🇷France prudloff Lille

I fixed the D10 test.

🇫🇷France prudloff Lille

@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.

🇫🇷France prudloff Lille

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.

🇫🇷France prudloff Lille

I merged the latest 11.x.

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

I created a new MR that targets 11.x.

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

🇫🇷France prudloff Lille

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.

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

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.

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

prudloff created an issue.

🇫🇷France prudloff Lille

prudloff created an issue.

🇫🇷France prudloff Lille

prudloff created an issue.

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

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.

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

prudloff created an issue.

🇫🇷France prudloff Lille

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.

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

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.

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

I rebased the MR.

🇫🇷France prudloff Lille

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.

🇫🇷France prudloff Lille

I rebased the MR.

🇫🇷France prudloff Lille

prudloff created an issue.

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

I fixed code style problems but now some unit test is failing.

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

I created a new MR based on 11.x but unit tests are failing.

🇫🇷France prudloff Lille

prudloff changed the visibility of the branch 3115188-routes-fail-validation to hidden.

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

Shouldn't this be handled directly by webform so that every exporter can benefit from it?

🇫🇷France prudloff Lille

Some tests are failing.

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

Can you still reproduce with the latest release?

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

Fixed by 🐛 Failing phpunit tests Active .
I think it is OK to drop support for Drupal <10.3. 10.2 is EOL since November 2024.

🇫🇷France prudloff Lille

I incorporated the fixes from 📌 Automated Drupal 11 compatibility fixes for config_override_warn Needs review and credited deimos.

🇫🇷France prudloff Lille

Hi, I am still interested in becoming co-maintainer.

🇫🇷France prudloff Lille

Some comments: rendering the toolbar is slow (mostly because of access checks)

I created a separate issue for this: 🐛 Rendering a toolbar with a lot of links is slow Active

🇫🇷France prudloff Lille

prudloff created an issue.

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

I merged the latest 11.x and tried to address comments on the MR.

🇫🇷France prudloff Lille

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

🇫🇷France prudloff Lille

I think the root cause is 📌 Support Yaml::PARSE_CUSTOM_TAGS in \Drupal\Component\Serial::decodeization\YamlSymfony:: Needs work . Before that, Yaml::decode() was not parsing custom tags.

Production build 0.71.5 2024