🇧🇪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?

Snapshot of current MR for composer

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.

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!

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

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

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

herved → created an issue.

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.

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

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

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.

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.

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.

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

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.

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

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.');
}

herved → created an issue.

#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?

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

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.

@joseph.olstad see 📌 Fix PHPStan issues reported on CI/gitlab Active and 📌 Fix PHPCS issues reported on CI/gitlab Active , thanks

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.

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.

herved → created an issue.

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

herved → created an issue.

herved → changed the visibility of the branch phpcs to hidden.

herved → changed the visibility of the branch phpcs to active.

herved → changed the visibility of the branch phpcs to hidden.

herved → created an issue.

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

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

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

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

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

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

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.

herved → created an issue.

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

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.

Snapshot of current MR for composer on 3.x

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

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.

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

Snapshot of current MR, for composer.

#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

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

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.

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

herved → created an issue. See original summary → .

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!

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?

Note that this introduced a regression, see 🐛 [regression] Entity view block not displayed after #2962166 Needs review

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

What is special about that content_moderation field, while other computed fields work just fine?

See #2321071-42: BaseFieldOverride fails to take into account ContentEntityInterface::bundleFieldDefinitions() when invoking onFieldDefinitionUpdate() → + this issue description.
It's convoluted...

herved → created an issue.

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

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!

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

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

+1, it looks ok to me.

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

herved → created an issue.

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.

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

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.

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.

herved → created an issue.

Does 🐛 Error: cannot call methods on dialog prior to initialization; attempted to call method 'option' Needs review solve this?

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.

It looks like the pipeline is currently broken on HEAD, as it was changed to use D11.

I noticed D11 requires at least two things:
- https://git.drupalcode.org/project/private_message/-/merge_requests/129/...
- https://git.drupalcode.org/project/private_message/-/merge_requests/129/...

herved → created an issue.

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   work?

Leftover again...
MR updated, and updated 2.2.0 patch

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

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