Account created on 19 July 2012, over 12 years ago
#

Merge Requests

More

Recent comments

🇧🇪Belgium herved

Ok my main question here relates to what should happen when we actually delete a node that has a redirect.

From #2879648-103:

For the node deletion problem:
- when a path alias is deleted,
- and a redirect for the source of the path alias exists
- if the existing redirect language matches the path alias language being deleted, we update the redirect source to the path alias url
- else if the existing redirect language == "und", we duplicate the redirect and set the redirect source and language according to the path alias being deleted.

Say we have an EN /node/123 with alias /foo
And a user created a redirect from /node/123 to /bar. At this point both /node/123 and /bar redirect to /foo.
Shouldn't we do anything when the node (and so path alias) gets deleted? so we maintain /foo > /bar redirection?

🇧🇪Belgium herved

Coming from 🐛 Redirects from aliased paths aren't triggered Needs review
I did some thinking back then on that issue (see comment #103 / #108) and I ended up with a similar conclusion to @berdir, which is that we should only inform/guide the user when they input aliases.
Till now I was using the patch from #104, but the approach here seems like an it improves on it. So thank you for moving it forward.

🇧🇪Belgium herved

Hello @cdteu,
We are preparing for the upgrade from Drupal 10 to 11, and tmgmt_cdt is one of the last modules blocking us.
Would you be able to take a look and prepare a release for D11 when possible?
Thanks in advance!

🇧🇪Belgium herved

herved changed the visibility of the branch 3509415-fix-deprecation to hidden.

🇧🇪Belgium herved

Thank you both, this is still a rough POC.
Indeed maybe using a "validated-member" OG role could be a good compromize for now.

🇧🇪Belgium herved

POC branch added, functional and kernel tests are passing, I only skipped unit tests.
I left a few todos.

My only major concern on this approach is that although the AccessPolicyProcessor caches things (statically and persistently) it can get quite expensive to load all memberships and groups to compute all permissions for a user, especially if he belongs in thousands of groups. Although that seems to be what drupal/group does...

🇧🇪Belgium herved

The IS is no longer accurate since 🐛 Make BaseFieldOverride inherit internal property from the base field Fixed fixed isInternal for BaseFieldOverride and path aliases are now exported.
But maybe we could use this issue to discard fields that are marked as read-only?
I created a MR, anyway.

🇧🇪Belgium herved

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

🇧🇪Belgium herved

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

🇧🇪Belgium herved

I discovered an issue in CommentLinksTest.php revealed by #2879087-148: Use comment access handler instead of hardcoding permissions
There are other cases in tests where the comment type is not created.

🇧🇪Belgium herved

I pushed an attempt now to address #118 to #125 (reply to {$pid}).
By calling $parent_comment->access('reply') in CommentController::replyFormAccess and doing the actual check in CommentAccessControlHandler::checkAccess

Also, I had a look at the other issue [PP-1] Add thread depth configuration to comments Postponed which requires this but it seems to rely only on CommentAccessControlHandler::checkCreateAccess at the moment. Since we now have the reply access operation, I'm not even sure we need all the changes to $context (parent_comment, commented_entity).
I'll sync with @claudiu.cristea on Monday.

---

PS: still the same 2 (less concerning) test failures which I left out for now since I don't quite understand them yet.

🇧🇪Belgium herved

Important fix for cacheability:
The CommentDefaultFormatterCacheTagsTest was failing because the CommentDefaultFormatter was not applying cacheability to the render array.

---

On that note, we have a need in our project where the create comment permission will have a very high cache variance (per user).
This means that currently, it would kill the Dynamic Page Cache when CommentDefaultFormatter checks for the create permission and adds the lazy_builder and cacheability to the render array.
I propose to move the access check inside the lazy_builder callback, in CommentLazyBuilders::renderForm.

I'd like to keep the scope to a minimum but this touches on the same lines and lots of projects may be in a similar scenario so we should try to preserve the page cache as much as possible. Viewing comment is less likely to have such requirements.
The change is very small, but this could also be reverted and a separate issue or follow up instead if anyone objects.

🇧🇪Belgium herved

For now here are patches of the current MR23 commit f2ac23e6.

🇧🇪Belgium herved

Update: I recreated the history from previous/2879087-comment-access-handler/2022-01-31 branch
https://git.drupalcode.org/issue/drupal-2879087/-/tags/previous%2F287908...
Merged 11.x, fixed conflicts, phpcs, phpstan and the tests touched by the patch.

5 tests are failing at the moment, among those these are concerning to me and relate to the changes in CommentController::replyFormAccess:
- core/modules/comment/tests/src/Functional/CommentNonNodeTest.php
- core/modules/comment/tests/src/Functional/CommentAnonymousTest.php

Also #137 may be right, I'll try to investigate those issues.

🇧🇪Belgium herved

Thanks @dieterholvoet,
I realize now that there might be an issue also with this part as it shows the user/pass in clear (placeholder) when we use config override:

if ($value !== $original) {
  $form[$child]['#placeholder'] = $value;
  $form[$child]['#disabled'] = TRUE;
  $form[$child]['#description'] = $this->t('This config cannot be changed because it is overridden.');
}
🇧🇪Belgium herved

#17 Hi @grimreaper,

Yes I also noticed these errors get triggered with CKeditor: https://git.drupalcode.org/project/drupal/-/merge_requests/9440#note_398735
Is this MR here fixing your issue?

There is also 🐛 Error: Cannot read properties of undefined (reading 'settings')
 with dialog.position.js Needs work which relates closely to this, maybe the patch there is is the proper fix?

🇧🇪Belgium herved

#189 I also encountered this issue during search_api indexing on a project with lots of languages (>20).
Because search_api processes each translation 1 by 1 and renders the rendered_item (Rendered HTML output) field which renders a view mode that contains a media.

🇧🇪Belgium herved

MR rebased, static patch attached:
- Deleted views.area.facets_area
- views.area.facets_summary_area and views.filter.facets_summary_filter should be now in facets_summary

Note that views.filter.facets_filter should now in theory be in facets_exposed_filters but is in facets.
I left it as-is for now.

🇧🇪Belgium herved

I created 2 branches so far:
- 3502957-fix-phpstan: this contains all phpstan fixes excluding DI, no BC breaking changes.
- 3502957-fix-phpstan-di: I started fixing some DI, but not all. I believe it breaks BC? pretty sure this won't happen anytime soon... Some of those are very easy fixes like \Drupal::messenger. Maybe a good way forward is to fix those easy ones and add inline phpstan exclusions @phpstan-ignore-next-line GlobalDrupalDependencyInjectionRule everywhere else. So we maintain BC, the CI will be green and new code has to be compliant.

🇧🇪Belgium herved

I'm going to close this as I'll go with a standard facets block approach or possibly the new native views filters in facets 3.0.0.

Note that the views plugins (area + trait) that were written in facets may be a good inspiration to improve this module, see:
- https://git.drupalcode.org/project/facets/-/blob/3.0.0-beta1/src/Plugin/...
- https://www.drupal.org/i/3073444
Note: this was later removed in 3.0.0-beta2 because facets maintainers decided to focus on native views filters from now on.

🇧🇪Belgium herved

MR created, note that this includes 🐛 Use of dynamic properties is deprecated in PHP 8.2+ Active , please credit users there as well, thanks

🇧🇪Belgium herved

herved changed the visibility of the branch phpcs to hidden.

🇧🇪Belgium herved

herved changed the visibility of the branch phpcs to active.

🇧🇪Belgium herved

herved changed the visibility of the branch phpcs to hidden.

🇧🇪Belgium herved

This no longer applies to 3.x

It looks like the facets_area views plugin was removed in 3.0.0-beta2, commit: https://git.drupalcode.org/project/facets/-/commit/c2f70843a5764450659bb...
And there were many other changes, moving back to needs work

🇧🇪Belgium herved

Sure, I can create an issue for phpstan.
Why excluding DI ? as it breaks backwards compatibility ?

🇧🇪Belgium herved

Sorry for the noise, I didn't know if I could rebase the existing MR, so I created a new one.
Tests are passing, the branch only needed to be rebased.
All credits to mohammad-fayoumi

🇧🇪Belgium herved

I'd be happy to help on those, but these are not strictly related to this issue are they?

🇧🇪Belgium herved

Converted the last patch #8 to MR, with some minor fixes added.

🇧🇪Belgium herved

It was just an idea, since I saw there was a lot of effort to remove all the jquery_ui dependencies recently.
Some modules also add a requirement message in the status page in case the library is missing.
And composer can handle library updates when using the npm-asset method (or composer-merge plugin).
Here is an example from js-cookie Support local library file Active

But feel free to close it if no-one else objects.

🇧🇪Belgium herved

Hello, what is the reason to make noUISlider a hard dependency in composer?
Couldn't we make it a soft dependency, or use the npm-asset method that a lot of modules are now adopting?
https://www.drupal.org/docs/develop/using-composer/manage-dependencies#t...

🇧🇪Belgium herved

Ideally it should also support future dates, resulting in a spike curve where the peak is NOW and decays in past or future.
In mysql at least, it looks like we can achieve the same formula as the one used in solr:
- solr: product(boost,recip(abs(ms(resolution,field_name)),m,a,b))
- mysql: boost * ( 1 / (m * ABS(TIMESTAMPDIFF(SECOND, FROM_UNIXTIME(field_name), NOW()) * 1000) + a) + b)

However I don't really know what would be the best to combine both numbers (keyword score that can be very high 1000-infinite), and this date boost. Maybe some kind normalization would be needed?
Something like: (score/(min_score+score)) where min_score = 1000
Then use a = 1, b= 0.1 producing a 0-1 date boost
and add both results?

I noticed that when keywords are given, we may need to join the main index table as search_api_db produces a group_by query which also makes things quite complicated.

🇧🇪Belgium herved

Snapshot of current MR for composer on 3.x

🇧🇪Belgium herved

Sorry for the late feedback, it seems to work well on my project and simplifies things greatly.
+1, thanks !

🇧🇪Belgium herved

Here is a static patch for 10.3.x, based on MR 8334.

The webform submission issue can be solved by applying 🐛 Incorrect use of 'render element' for webform_submission_data Active on webform 6.2.x, or updating to 6.3.x.
Adding revision and language to the key like MR 4994 does would make sense I think.

🇧🇪Belgium herved

Oh indeed, that could simplify things a lot.
Maybe loading all DS layouts and setting the default type to ds.layout_plugin.settings.default

🇧🇪Belgium herved

#202, FWIW on ddev I create a file .ddev/php/xdebug.ini and put this:

# Do not use xdebug develop mode causing transaction timing issues.
# @see https://www.drupal.org/project/drupal/issues/3405976
xdebug.mode=debug
🇧🇪Belgium herved

herved changed the visibility of the branch 3456722-regression-entity-view to hidden.

🇧🇪Belgium herved

I pushed a better/proper recursion detection logic, with a similar logic than 🐛 Can only intentionally re-render an entity with references 20 times Needs work .

PS: the pipeline on 3.x is currently broken.

🇧🇪Belgium herved

Update: we also hit the webform submission issue (thankfully from our tests) as mentioned in comments 163 - 179
I created an issue in webform and proposed a fix there 🐛 Incorrect use of 'render element' for webform_submission_data Active

🇧🇪Belgium herved

I encountered such issues with Media but this could also solve issues such as 🐛 [regression] Entity view block not displayed after #2962166 Needs review since it applies to all entity types.
Finally a true recursion detection and using pre/post_render callbacks to do it is very interesting.
I tested it on one of our projects and it works great, thanks!

🇧🇪Belgium herved

Note that the recursion detection is still not very good.
Maybe we could borrow the same approach as 🐛 Can only intentionally re-render an entity with references 20 times Needs work ?

For now couldn't we revert the changes from the issue that introduced this regression?

🇧🇪Belgium herved

Short explanation:
- RdfSyncExceptionSubscriber::onException transforms any non-http exception into an http one
- it also transforms a NotEncodableValueException (supposedly coming from the normalizer) into a NotAcceptableHttpException which gets logged as a client error right after in \Drupal\Core\EventSubscriber\ExceptionLoggingSubscriber::onException

🇧🇪Belgium herved

#54, see #37, the current patch only prevents moderation_state BFOs to be exported. Existing ones need to be removed manually.
Let's not continue with patches and create a merge request.

🇧🇪Belgium herved

Tested this on our project at /admin/people/permissions, using network tab:

twig v3.14.0:
- after cr: 3.29s
- next load: 2.58s
twig v3.15.0:
- after cr: 8.39s
- next load: 7.60s
twig v3.15.0 + patch from https://www.drupal.org/i/3487031:
- after cr: 3.27s
- next load: 2.79s

In practice, I did not notice any performance degradation at first. Probably because the form has to be very complex, and the browser itself (UI performance) has to keep up anyway for example for this permission page. But there seems to be quite an impact indeed on a pure PHP performance. This may also behave worse on different environments as others stated.

I also ran the same tests with xhprof and the results are telling, it goes from 3s to 46s to load the page, with a very high wall time on Twig\Extension\SandboxExtension::ensureToStringAllowed.
So +1, looks like it fixes it, thank you!

🇧🇪Belgium herved

I tested the above scenarios, works correctly. Just a minor nit.
+1, looks like a good fix and maintains BC.

🇧🇪Belgium herved

+1, looks ok to me
Maybe the scope of 🐛 Remove private-message-* id Active could have been combined here, but it's fine either way.

🇧🇪Belgium herved

PS: Do we still need to support Drupal 9 (from info.yml)?

🇧🇪Belgium herved

I just stumbled on this issue, in our project we use symfony_mailer_lite and print {{ subject }} in the twig mail template.
Twig encodes these chars to HTML entities: '&<>
If the subject contains any of these chars, then they get printed as encoded in the final twig template/email.

This is not an issue for the actual mail subject as it goes through PlainTextOutput::renderFromHtml() here.
But the twig template is rendered just before that, from here.
So I believe it makes sense to apply the proposed suggestion.

🇧🇪Belgium herved

Changes look good to me, thanks.
I did some manual tests and didn't any other issues/regressions.

🇧🇪Belgium herved

We can reproduce this as follows:
1. Enter in "to" field: foo,bar,baz
2. Remove all 3 user items
3. Redo step 1 again, we get an AJAX error

Attaching a gif to showcase issue.

🇧🇪Belgium herved

Can't we simply use $private_message->get('message')->processed ?
This uses \Drupal\text\TextProcessed::getValue and returns a FilteredMarkup which would align with the filter format. Xss::filter may filter way more than expected.

🇧🇪Belgium herved

Is the ??[] needed here $this->config->get('form_settings') ?? []? I believe form_settings is always an array.

Oh good point, indeed, otherwise it would have nullable: true in the config schema if I'm not mistaken.
MR updated.

🇧🇪Belgium herved

Indeed this doesn't seem valid: https://jsfiddle.net/cafsjowu/1/
It looks like 📌 Prevent extlink icons from breaking into new lines Needs work recently committed added an extra span wrapper in the mean time?
Can you please create a new issue @cboyden? thanks

PS: The added code there looks convoluted. I wonder, wouldn't simply &nbsp; work?

🇧🇪Belgium herved

Leftover in HoneypotSettingsForm::submitForm...
MR updated, and here's the updated 2.2.0 patch

🇧🇪Belgium herved

+1 with avoiding this in the future, this is a BC break and needs a major release.
Having to pin/unpin in composer is tedious.
If every module were to do that, it would be quite a hassle.

Production build 0.71.5 2024