🇧🇪Belgium @herved

Hi @joelpittet, I know I combined 2 issues into 1 here, which may not be ideal, but they are tighly coupled.
In my understanding, considering this issue here, and the changes I propose, external files would and should never enter the ::optimizeGroup method. So does it really make sense to check for minified === TRUE && type === external in ::optimizeGroup as you proposed there? Meaning the condition is therefore not needed.

Static patch for composer.

Hello, if you are hitting "Only file JavaScript assets can be optimized" or "Only file CSS assets can be optimized" I just created 🐛 Assets paths in CSS no longer rewritten when aggregation is enabled Active which looks like a different case than the one reported here.

Hello, thanks for creating this issue.
I did hit this as explained in #3414173-37: Add support for minified external CSS libraries → (comments 37-39).
But I also hit another issue which relates to 🐛 Only file JavaScript assets with preprocessing enabled can be optimized. Active
Both are closely related so I opened 🐛 "Only file JavaScript/CSS assets can be optimized" errors in logs Active and proposed a slightly different approach.
Any feedback is welcome :) Thanks

Tests and steps to reproduce still todo.
But any feedback is welcome.

Opened MR
For context, I suspect we may have an infrastructure issue somewhere, possibly the reverse proxy serving stale pages.
Still, if that is the case, drupal should not fill the logs with errors. The changes here will log BadRequestHttpException warnings just like other invalid asset requests.

herved → created an issue.

Attaching static patch for composer.

Hi, what is the purpose of this use_form details? just purely UI to put elements under this collapsible section?
If so I proposed a solution for this at [3452930-8] by using a #pre_render instead of #validate.

I noticed a few issues and opened MR to address those https://git.drupalcode.org/project/spamspan/-/merge_requests/37

- At the moment the formatter settings with layout builder are not saved correctly and values under use_form are nested and not flattened. This happens because the form structure with layout builder is different than with field display overview form so the code in EmailSpamspanFormatter::validateSettingsForm is not correct.
- To solve this I removed this validate handler and use a #pre_render to display the use_form details while keeping the values flat.
This way it works on both and we do not need to know the entire form structure or make conditions.
- Added a test for the spamspan formatter and config schema tests for that and filter format
- Also centralized config schema under spamspan_plugin_settings so we can avoid repetition use it for the formatter and filter.

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

Updated IS, added MR and test coverage (the test fails correctly on D11 without the fix).
Adding also static patch for composer.

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

Created MR, took the patch from #67 (2912092-67-10-1-x.patch), fixed eslint, and one twig missing commas.
Attaching static patch for composer.

Tests still needed. I had a look but we cannot simply replicate in \Drupal\FunctionalJavascriptTests\Core\Form\JavascriptStatesTest, this needs fields on an entity form for every scenario (e.g.: multi-value fields, date fields,...). Also media library fields if possible, which is why I need this patch.

I rebased MR, fixed phpcs/phpstan and converted the hook to OO, tested on demo_umami and it looks fine.
Added 1 question in MR, this needs test coverage, so leaving to needs work.

Attaching also static patch for composer.

phpcs fails, some more work needed

I rebased the MR and added test assertions to address #47.
Attaching static patch for composer that applies on 10.2.x.

MR !12521 seems to solve things partially.

Maybe the inline_form_error deserves its own issue, not sure...
But many errors are attached to the field itself (not properties) and in such cases inline_form_error links points to anchors that do not exist in the form. This happens on single element fields.

To me it's very strange and suspicious why field-multiple-value-form.html.twig does not print the outer div when not multiple, along with attributes and #id of the element. So something like
{% if multiple %}
...
{% else %}

{% endif %}

This may be a decent way to solve that issue.

@joelpittet yes I'm sure it's this commit and reading the code, it makes total sense.

IIUC the only thing \Drupal\Core\Asset\CssOptimizer::optimize does is to fix/rewrite URLs when aggregating, which is always welcome for CSS IMO. Maybe this is why it didn't match the same logic as JS?
IDK but I suspect this is going to cause issues for others as well when upgrading to D11.2, at least if they used minified: true and require url() in their CSS to be rewritten.

This change seems a bit unexpected to me.
I didn’t read the full issue yet, but I have a theme that compiles and minifies SASS to CSS using Webpack, so I was using minified: true, assuming it would prevent additional minification by core.

After upgrading from D10 to D11.2, I noticed that url() references in my @font-face declarations are broken when CSS aggregation is enabled.
In 11.2, the CSS files retain relative url() paths, regardless of whether aggregation is enabled — which wasn’t the case before.
For now, I’ve removed minified: true from those files, even though they’re already minified by my build process.

Was this behavior change intentional?

Static patch for composer

So far I see the code at core/modules/contextual/js/contextual.js:167 is attempting to get data-contextual-id elements that are not guaranteed to be there if these were relocated.

window.setTimeout(() => {
  initContextual(
    $context
      .find(`[data-contextual-id="${contextualID.id}"]:empty`)
      .eq(0),
    html,
  );
});

But this line hasn't changed in #3203920 so probably there is something more at play there.

herved → created an issue.

herved → created an issue.

This patch can be combined with 🐛 PHP warnings from FileUrlWidget Needs review which fixes another error with the widget.
Maybe it's best to centralize both in a single issue?

I opened a MR for the 2nd option: replace the URL autosubmit and JS with a "Add URL" button.
Because this is a much better approach for UX: an autosubmit on input change is very intrusive for the user and a button is a much better approach.

I did have to refactor the #access applied on elements because having it to FALSE causes havoc.
It seems in that case Drupal doesn't understand what button triggered the action in \Drupal\Core\Form\FormBuilder::handleInputElement and considers the first button in form_state was clicked. This causes all kinds of misbehaviors.

- Installed latest core 11.x
- Enabled dependencies (jquery_ui, jquery_ui_draggable, jquery_ui_resizable) + bootstrap theme 3.x-dev
- Tested the navbar hamburger on mobile and collapse, all ok
- I can see both fixes from entreprise7pro/bootstrap are present
https://github.com/entreprise7pro/bootstrap/commit/00ee4669f6e24c3635a2b...
https://github.com/entreprise7pro/bootstrap/commit/51adf3b32846a78f3b8e7...
- +1 LGTM

Noticed the same after updating from core 10.4 to 11.2.
This is a regression from 🐛 Prefix/Suffix not inline with autocomplete field Active and happens at least with multiple cardinality link fields.
Discovered and closed a duplicate issue ✨ Multi-Link Fields styling applying flex class Active .
Updated IS and title

The current MR in both issues seem to break/ignore what the parent issue meant to achieve.
Since I'm clueless in CSS, here is a patch that reverts the changes from #3471459.
Hopefully someone can provide a proper fix.

This is a duplicate of 🐛 Link field description squashes the field Active which is older so I assume we can close this and centralize efforts there.

Current MR snapshot, for composer.

Created new MR, the actual fix is in OpenDialogCommand::__construct
- 📌 Removal :tabbable usage in dialog.js Fixed was committed to 10.3.x and 11.x but their commit differs, mainly in OpenDialogCommand::__construct where the issue lies in 11.x, because if $dialog_options['dialogClass'] is set (it is via ckeditor5.js), then $dialog_options['classes']['ui-dialog'] is not taken into account.
- this MR contains improvements in order to ensure both $dialog_options['classes']['ui-dialog'] and $dialog_options['dialogClass'] are taken into account/preserved.
- 10.3.x is not affected by this bug per-se because it does preserve $dialog_options['classes']['ui-dialog'], however it could (should IMO) be ported as well, for consistency.
- I took the opportunity to align the way this is handled in OpenDialogCommand and OpenOffCanvasDialogCommand
- Added some regression tests for those + test to ensure the media-library-widget-modal class is present on the ckeditor media library modal.

herved → changed the visibility of the branch 3474018-alt to active.

herved → changed the visibility of the branch 3474018-alt to hidden.

I can confirm the issue as I stumbled on this after upgrading to D11.
It's very easy to reproduce with demo_umami profile and opening the add media modal from ckeditor.
I just did a git bisect on the 11.x branch and
66c3914d39bf24d828ec8516389d7cadad52294e is the first bad commit and indeed comes from 📌 Removal :tabbable usage in dialog.js Fixed

This ultimately needs maintainer decision but +1, looks good to me.
Without this patch we cannot have a tagify select field without this description.

I tested manually both forms, adding a button that rebuilds the form, no errors anymore.
+1 LGTM, thanks

I was able to reproduce easily, and the proposed changes are fixing the issue.

I just left a comment regarding the whole FieldConfigFormAlter/BundleEntityFormAlter/AbstractFormAlter/FormAlterInterface setup which is convoluted and unnecessary IMO, and could be done in just 1 service class, which may result in a more flexible architecture.
But this is probably OOS, and needs maintainer feedback.

I think there are issues with the approach of hiding form elements with CSS.
In our case a textarea element in the form has the hidden class but another class (form-element) applied on it overrides the display so the element is visible on all steps.
Also, doing this means that it completely changes the validation as it requires all required fields in next steps to be filled in while the original implementation of this module does not.

entity_form_steps has an interesting take on it, where they preserve values when going back but only keep the valid ones.
See EntityFormSteps::validatePreviousForm. This would involve also changing #limit_validation_errors and simple_multistep_register_back.

herved → created an issue. See original summary → .

Static patch for composer

I tried to add a phpunit test, but ConfigSchemaChecker::onConfigSave does not validate the constraints for non-core tests, see \Drupal\KernelTests\KernelTestBase::register

herved → created an issue.

Static patch (MR snapshot) for composer.

herved → created an issue.

Hello, for formatters it looks like the ui_patterns_source.icon schema is missing.
Attaching static patch as well.

herved → changed the visibility of the branch 3493153-config-schema-fix to hidden.

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

Thanks @amitaibu, should we also backport this to 1.x? since the issue is there also.

Sure, I added a very basic test in GroupMembershipManagerTest.
It fails as expected if we revert the fix (see test-only-changes job).
Hope that's good enough.

I propose to extend the scope slightly and add some safety checks in the sevice methods to return early in case the $group entity is new/has no ID yet.

herved → created an issue.

herved → changed the visibility of the branch 3262762-field-widget to hidden.

I created 2 branches:
- 3262762-field-widget that uses third_party_settings
- 3262762-field-widget-plugin that defines a new widget plugin extending options_select.

I think the widget plugin option is way better.
We do not have to deal with hooks and a dedicated widget will not pollute all options_select configs. Using third_party_settings, the slim_select dependency and third_party_settings would be exported for all options_select configs, even when unused, which is very annoying.

Also, can we drop 10.2 support (which is EOL) so we can use attributes and not annotations (included since 10.3, https://www.drupal.org/node/3229001 → )

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

(note: I am not a maintainer but) I do have some architectural concerns as well.

What happens if the jobs are purged? See tmgmt_cron().

This, also jobs can be deleted manually.

Currently, the block shows the message if there is an accepted/completed job item for the given entity type, ID and language.
But what about the revision? should that be taken into account?
Also, what if the entity was translated with tmgmt but using another source plugin?
To me this looks more like some business logic that could vary greatly depending on the site.

Maybe this can be implemented as a base field (revisionable, translatable) in the entity.
But of course it would need to be set on the translation upon save. I did find ✨ Hook to allow modules to alter translation entity being created RTBC which may help on that.

Ok thanks for the clarification. I see now this is only the case with the upload method but we use the path one.
So in this case I guess making the path relative is one option.

herved → created an issue.

herved → created an issue.

I just stubled on this, the current starterkit setup on radix 6 includes the main css file, which is rather bad as it will leak outside ckeditor.
Also I don't understand how #4 B and #16 could work because laravelmix executes everything in parallel. If I delete my build folder, and try to mix.postCss a file that is generated via mix.sass, laravelmix complains that the source file doesn't exist (because it has not been compiled yet).
Where is assets/css/ckeditor5.style.css coming from there?
Processing the same entry twice also is not an option because mix complains about that.

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.