🇳🇿New Zealand @ericgsmith

Thanks team!

I've tested on 11.x - confirmed that when config_translate is not enabled with MR38 there are no issues or errors enabling / using the module. After enabling config_translate the translations specific routes / UI are present and appear to be functioning as expected - so it looks like no regression here for people with config translate enabled.

Code changes look good - phpstan is complaining "Ternary operator condition is always true." but clearly that is not the case - can ignore that one.

Setting as RTBC

Restoring the previous RTBC status - #63 was providing comment / fix to patch 45 which was more than 2 years older than the MR - it has been already fixed a long time ago. 7cbb399fbcd6a589a9665c17ed57bfdc697bd876 just introduces formatting change. Acknowledging that tests currently fail as previously mentioned in (#62).

We have been using this patch for several years now and now on multiple projects where we have encountered similar issues to the many described in this thread, it would be awesome to see this committed.

Hiding all patches to hopefully prevent further confusion on the issue.

MR36 is a good work around for people impacted by this (the other work around being to enable content translation).

I'm setting this to needs work though as I think the issue summary still stands that the module should either list config_translation as a dependency, or properly adjust the code to not make it required. By properly adjust the code - I think that means check appropriately that it is only adding dependencies to config translation when the module is enabled. MR36 current reverts the feature entirely.

My preference is that config translation remains optional - I'm not sure if there is some prior art from other modules we can take inspiration from here? Seems like initial changes from @pasqualle were heading in that direction.

Linking related issue - this created a critical regression for sites not using content translation.

I applied the suggested after testing it - I note above to review it then apply it, but to move I've applied it so I can set to Needs review.

I repeated my manual tests above and the upgrade path is still work, and the issue I found in 168 is resolved although somebody else would need to verify that.

Manual test - 11.2.x => 11.3.0-rc1 => 3274635-add-missing-update-path-if-using-CKE5-list-plugin-only at 2f315be

Config before upgrade:

$ drush config:get editor.editor.list_only settings.plugins.ckeditor5_list
'editor.editor.list_only:settings.plugins.ckeditor5_list':
  properties:
    reversed: true
    startIndex: true
  multiBlock: true

$ drush config:get editor.editor.basic_html settings.plugins.ckeditor5_list
'editor.editor.basic_html:settings.plugins.ckeditor5_list':
  properties:
    reversed: false
    startIndex: true
  multiBlock: true

After update to 11.3.0-rc1

$ drush config:get editor.editor.list_only settings.plugins.ckeditor5_list
'editor.editor.list_only:settings.plugins.ckeditor5_list':
  properties:
    reversed: true
    startIndex: true
  multiBlock: true

  $ drush config:get editor.editor.basic_html settings.plugins.ckeditor5_list
'editor.editor.basic_html:settings.plugins.ckeditor5_list':
  properties:
    reversed: false
    startIndex: true
    styles: true
  multiBlock: true

After update to 3274635-add-missing-update-path-if-using-CKE5-list-plugin-only at 2f315be

$ drush config:get editor.editor.list_only settings.plugins.ckeditor5_list
'editor.editor.list_only:settings.plugins.ckeditor5_list':
  properties:
    reversed: true
    startIndex: true
    styles: false
  multiBlock: true

  $ drush config:get editor.editor.basic_html settings.plugins.ckeditor5_list
'editor.editor.basic_html:settings.plugins.ckeditor5_list':
  properties:
    reversed: false
    startIndex: true
    styles: true
  multiBlock: true

After enabling the styles option via UI it saved correctly and is now present:

$ drush config:get editor.editor.list_only settings.plugins.ckeditor5_list
'editor.editor.list_only:settings.plugins.ckeditor5_list':
  properties:
    reversed: true
    startIndex: true
    styles: true
  multiBlock: true

Setting to needs review for the remaining tasks from #169:

• The latest MR suggestion needs review .
• Then it needs a full round of manual testing again.
• If an upgrade path test is also added, that would be best (not an absolute must have).
• Probably also need a follow-up to add tests for the UI step @ericgsmith found.

I can confirm the upgrade path is working for the scenario where the config was missed (in addition to verifying steps from 164/1165) - however there is an additional bug introduced now where the config can not be correctly updated after the update hook has run.

Update hook working:

1. Checked out 11.2.x HEAD and installed Drupal using standard profile
2. Create a new text format using CKEditor5 as text editor, with the "Bulleted List" button enabled and source editing not enabled:
   

   $ drush config:get editor.editor.test_list_only
   uuid: f84bfb98-2071-44d8-9ef0-e7544358422e
   langcode: en
   status: true
   dependencies:
     config:
       - filter.format.test_list_only
     module:
       - ckeditor5
   format: test_list_only
   editor: ckeditor5
   settings:
     toolbar:
       items:
         - heading
         - bold
         - italic
         - bulletedList
     plugins:
       ckeditor5_heading:
         enabled_headings:
           - heading2
           - heading3
           - heading4
           - heading5
           - heading6
       ckeditor5_list:
         properties:
           reversed: true
           startIndex: true
         multiBlock: true
   image_upload:
     status: false

3. Checked out 11.3.0-rc1 and updated to it
4. Ran the update and confirmed that "Updates Text Editors using CKEditor 5 to native List "type" functionality" was run.
5. Observed the issue - full html and basic htl editor config has correct config, test_list_only is missing as per comment #160
   

   drush config:get editor.editor.test_list_only settings.plugins.ckeditor5_list
   'editor.editor.test_list_only:settings.plugins.ckeditor5_list':
     properties:
       reversed: true
       startIndex: true
     multiBlock: true

6. applied patch from MR14034 and confirmed update ran again
7. Confirm config missed from the initial update is now as expected:
   

   $ drush config:get editor.editor.test_list_only settings.plugins.ckeditor5_list
   'editor.editor.test_list_only:settings.plugins.ckeditor5_list':
     properties:
       reversed: true
       startIndex: true
       styles: false
     multiBlock: true

New bug

After the update hook is run I edit the format to enable the plugin as per the change record:

Sites without text formats currently allowing the type attribute can enable it manually using the "Allow the user to choose a list style type" option from the List CKEditor 5 plugin settings:

1. Edit the text format
2. Check "Allow the user to choose a list style type"
3. Save
4. Return to the edit form, value is unchecked and styles is not enabled

What is happening is that the pre save hook runs, source editing is still disabled so the else if block array_key_exists('ckeditor5_list', $settings['plugins']) returns true and the config gets reset to FALSE again despite the user enabling it.

I think we need to change to something like this similar to the check on L425:

      elseif (array_key_exists('ckeditor5_list', $settings['plugins']) && !array_key_exists('styles', $settings['plugins']['ckeditor5_list']['properties'])) {
        $settings['plugins']['ckeditor5_list']['properties']['styles'] = FALSE;
      }

ericgsmith → created an issue.

Config entity scheme looks right now, still need to add the schema for the field formatter

Fixes the issue, change looks good

Expanded the scope to some other warnings I picked up

ericgsmith → created an issue.

The schema changes look good - but I think maybe it needs to filter out the values before saving?

E.g I see from config saved before the patch applied I have:

media_types:
  document: document
  image: 0
  remote_video: 0

When I resave it changes this to:

media_types:
  document: document
  image: '0'
  remote_video: '0'

When really we want those disabled types filtered out.

Composer job failure looks unreleated

ericgsmith → created an issue.

ericgsmith → created an issue.

Might get the odd schema warning for disabled text formats still with this - e.g. full_html: 0 - would get resaved as '0' eventually.

Probably better to fix up the config so that it just exports the enabled formats?

ericgsmith → changed the visibility of the branch 3534861-schema-errors-for to hidden.

It is not that the config is missing from the schema, it is that when saved through the UI this form item is being treated as configuration. Its a read only bit of markup - maybe the issue lies in the submit handler?

https://git.drupalcode.org/project/openid_connect_windows_aad/-/blob/2.0...

Looks like the 8.x 2 branch was actually tagged off 1 so it's a bit confusing, moved patch and fix to mr and opened against default branch

+++ b/config/schema/ckeditor_templates.schema.yml
@@ -21,8 +21,16 @@ ckeditor_templates.ckeditor_templates.*:
+      type: mapping

Sequence was correct - was just missing the sequence type, e.g.

formats:
  type: sequence
  label: Available for
  sequence:
    type: string

https://git.drupalcode.org/issue/ctools-3464574/-/jobs/7530512#L110 shows test only change - looks good to me and resolves the config errors - good work.

And test failing as the logic for usage is broken with my changes if the file entity has multiple usages that get deleted at the same time like in the tests

Added draft MR just as a rejig of the order to get it working locally. Tests still pass but if this would need additional test with trash module enabled. Open for different approach too, was just first trying to get something working.

ericgsmith → created an issue.

Doing a bit more research, Broadcast Channel API looks to have pretty good support and seems ideal for this kind of situation.

https://developer.mozilla.org/en-US/docs/Web/API/Broadcast_Channel_API

I think a lot of the discussion above centers around the idea that we have an active tab. We have no guarantee that the last tab to have activity will be the tab they go back to - e.g. I might have Drupal open in 2 windows, leave it in one window, open another application then go back to a different window.

I think we need a solution so that no matter what tab I come back to I can see the dialog if it should be visible.

I think the removal of the activity time in #32 is problematic.

Testing in just 1 window, after doing activity to 30 seconds - the original timer first at 60 seconds instead of being reset whenever there is activity. So at 60 seconds the isUserActive returns true and the timer is reset - e.g. will now appear at 120 seconds. So instead of appearing 60s after the last timeout, its now appearing 60s after the initial load.

I wonder if to resolve the issue first reported in #32 something could be done with storage events? E.g. go back to MR70 for setting a bool value, but move the logic for clearing the timeout and setting a new one to a storage event listener, that way it would run in all tabs?

Thanks @krystianbrzoza - can confirm this is still working as expected. Thank you

Confirm this is happening on client sites with Drupal 10.5.x and 11.2.x - I can not reproduce this in 11.x / 11.3.x though - I suspect something in CKEditor was fixed (11.2 is v45.2.0, 11.3 is v47.1.0)

Couldn't see anything specific related to this in the changelog though - maybe somehow impacted by https://github.com/ckeditor/ckeditor5/issues/19193

https://git.drupalcode.org/issue/drupal-3560357/-/jobs/7465803#L57 test only change shows the issue.

After adding the test I see there is already an existing TaxonomyDefaultArgumentTest just testing a view title - I guess we should move this view and test into that existing test rather than a new one. Happy to do that, will wait for a feedback on the change and test itself first.

ericgsmith → created an issue.

Should this be reverted - or open a new issue to revert? As mentioned about - this does nothing and the issue still persists. #3091671 looks related but not general enough that it would solve this issue. I will open a new issue in a couple days unless the etiquette is to reopen this

Thanks @krystianbrzoza - yes sorry I didn't mean to sound discouraging - yes the work has been done and it is good work. I just meant in the context of a maintainer needing to review this, keeping the issue focused and limited in scope may have be easier to review. I'm not talking about not doing the work, but splitting it across issues to keep the reviews smaller.

Anyway - as you say the work is done - if you are able to fix or add to the baseline those last phpstan issues I can review - @rosk0 became maintainer to get the v2 / d9 release done years ago - I can ping him once we have everything ready here.

Updating issue status to reflect latest pipeline failure - IMO we can drop that last commit and open up follow ups for oop hook conversion and if needed the tightening of phpstan rules, but I think up to maintainers if they want project specific phpstan config over the gitlab ci defaults.

@krystianbrzoza I was just about to comment that I think this is good to go but I see you are still working on it - introducing oop hooks etc. Just be mindful of expanding the scope beyond what we need here - I'll jump out again but can review when you are done.

Have rebased it.

Now that content_moderation.module file has been removed in 11.x we needed somewhere to move what was added as content_moderation_save_workflow_actions - I moved this to the hooks class. Would probably be better to move to a service.

I also don't think it needs to be exposed.

Install hook - probably should be post update? As the hook logic is now internal I've modified that somewhat. I see this was actually done previously but only as a patch and not to the MR.

Unit test - seems flaky - the test looks to have been failing for a while - we are just asserting something by telling to mock to provide that thing... I dunno, seems like its really complicated to read that unit test.

Still needs work -

1) /builds/core/modules/content_moderation/tests/src/Unit/Plugin/Action/ModerationStateChangeTest.php:126
Creation of dynamic property MockObject_NodeInterface_60cafd67::$moderation_state is deprecated

https://git.drupalcode.org/issue/drupal-2797583/-/jobs/7427863/viewer

Also - some changes have been added to 10.4.x instead of the main 11.x branch - I can try tidy this up tomorrow. I'm not so keen to keep battling with the unit test though.

Re #230 - yes there is a need to keep them. We never depended on the action UI in this mr and that doesn't change now that its removed.

ericgsmith → changed the visibility of the branch 2797583-dynamically-provide-action-9.4.x to hidden.

ericgsmith → changed the visibility of the branch 9.2.x to hidden.

ericgsmith → changed the visibility of the branch 2797583-dynamically-provide-action to hidden.

I think given how long this is committed I don't want to revert it without providing a way to keep it for people who need it - added draft MR for myself to patch it back out.

All targeted issues are now fixed.

There is a postponed issue and one open to improve documentation. I think documentation has been improved to the point it can - I think if there is any future effort available it would be to improve the config itself so that it makes more sense, instead of the documentation having to do more work.

For now at least the examples in the README are accurate and the tests show some variety. Lets get this out.

!11 was merged 2 years ago

So we can't auto add the space as the select-* classes depend on there being no space.

While config appears to have been fixed outside of this MR a while ago the update from advancedformsettings to settings is still a good cleanup and makes sense.

Rebased the MR + fixed docblock for the update function looks good to me

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

ericgsmith → created an issue.

I have added some test coverage for this. This just checks the parsing behaviour - not the full end to end export.

Passing: https://git.drupalcode.org/issue/config_profile-3558402/-/jobs/7345781

Test only job showing the issue: https://git.drupalcode.org/issue/config_profile-3558402/-/jobs/7345782

I believe the removing of optional uninstalled config may be an unexpected behaviour here - as a lesser of 2 evils I think duplicating the config into install is worse. I think the delete functionality might be able to be improved or handled better in a different way outside of the scope of this issue - welcome feedback from maintainers on this one.

Setting to needs review.

Setting this to needs review although I'm really not sure if we should be passing timezone as an option or data - maybe a token expert can weigh in here.

Either way - I think this demonstrates the issue well and hopefully not too far away from a fix - happy to keep pushing this forward if somebody can please review.

Pipeline with test + fix passing: https://git.drupalcode.org/issue/token-3317688/-/jobs/7344220

Test only changes failing demonstrate the issue: https://git.drupalcode.org/issue/token-3317688/-/jobs/7344224

Cool - test added to show the issue fails as expected: https://git.drupalcode.org/issue/token-3317688/-/jobs/7344007#L263

Bitten by this bug today.

Good summary of the issue.

If we look at what core is doing it adds a check for the type when setting the timezone https://git.drupalcode.org/project/drupal/-/blob/11.2.8/core/modules/dat...

I believe around here https://git.drupalcode.org/project/token/-/blob/8.x-1.16/token.tokens.in... we should add a check for the type on the field definition - and if it is date we could add an additional timezone - perhaps as an additional option? I'm not familiar with the token module enough, but some way of passing this along to the generate method would be needed.

Then we should just need to use it here https://git.drupalcode.org/project/token/-/blob/8.x-1.16/token.tokens.in... instead of passing NULL as the timezone.

Will do a bit of hacking around see if I can get something working

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

ericgsmith → created an issue.

Duplicate of fixed issue 📌 Automated Drupal 10 compatibility fixes RTBC

Committed at #8 - just closing off issue

ericgsmith → created an issue.

Thank you project update bot!

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

Resolved thread on original feedback - think the existings eslint issue is out of scope for this.

Test passes on D10 - this looks good to go

Given the phpcs job is green I am closing this as outdated and transferred credit to https://new.drupal.org/contribution-record/10034469

Apologies I should have reviewed and merge here when we applied 📌 Add .gitlab-ci.yml Fixed - hence transferring credit there.

For the MR - it looks like some of the changes were applied when Gitlab CI was introduced.

Other changes (e.g. only supplied patches) not pushed to the MR I have not reviewed.

Important things to note:

Latest pipeline shows phpcs passing - https://git.drupalcode.org/project/sector_multipage/-/pipelines/661883

PHPCS should not be used to check CSS and JS - there are other tools for that.

Currently the phpcs job does not scan the markdown files - I would be keen to wait for the CI to show us issues before applying changes to that.

The eslint job is failing - but might be out of scope for this issue unless we want to make this a bit more generic.

I tried to refactor MR6 and went on a huge tangent doing some refactoring - out of scope for this issue but we are where we are now.

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

I believe this is good to go - I will wait a few days to get feedback on update hook #9 otherwise I can merge and tag a new release next week.

Have made an adjustment to the gitlab CI file so that cspell is green and its testing against 10 and 11.

@gaurav.kapoor I believe we don't need an update hook here. Existing config would have been taken care of by views_post_update_remove_default_argument_skip_url - I have tried and install on 11 at 71ead06d2e39495e70de46b94d99f62d6e8254fd (before the commit to tidy up the config) and when exporting the config I do not see the default_argument_skip_url - presumably as when installing its not part of the schema so is ignored.

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

ericgsmith → changed the visibility of the branch 3169876-better-handling-of to hidden.

@dqd thank you for the getting the ball rolling on this one 🙌

I've made an assumption here the needs work related to getting the CI jobs green.

I've made some minor changes to get cspell and phpcs happy.

I dropped the drush requirement from the composer file - this was preventing CI from installing - I don't think this should be specified by the module as there does not appear to be any drush integration.

I added a basic test using the steps described on the project description to validate that #5 is not an issue. This could probably be optimised to be a kernel test - but given its the only test in the module and the code doesn't change frequently I'm not too concerned about the speed.

I think this is ready for review now

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

Added test coverage and gitlab ci file

Second thoughts, this is small enough that we should have test coverage - will include gitlab ci file and see if we can get green tests here

Bumped version number.

Tested in 11.x with no issues - evidence in https://www.drupal.org/files/issues/2025-11-09/Recording%202025-11-10%20... →

ericgsmith → created an issue.

- Applied gitlab ci template
- Fixed eslint issues
- Fixed the automated phpcs issues, this job still fails with a warning - could potentially solve these here but its fairly long manual review of all the comments and functionality to fix, I'm not comitting to doing this at this point
- Fixed 1 phpstan issue and comitted the baseline to ignore the 2 di rules - again could be done in a follow up if needed

ericgsmith → created an issue.

Opened MR with the change and tested on a standard install of Drupal 11.x

No issue found when configuring download options and field formatter.

No issue found viewing and downloading the files.

There is no Gitlab CI setup for this project or tests - I will open a separate issue for this as it would be good to prove the compatibility with a test.

ericgsmith → created an issue.