Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
Account created on 26 December 2006, about 17 years ago
#

Merge Requests

More

Recent comments

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

@catch in Drupal Slack:

I think we should and can get rid of the UI in a minor. Will need a change record/release note.

So: let's do that! (Separate change record for the UI change πŸ™)

@dimitriskr: Are you interested in making that part happen? :D

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Another DB query bites the dust β€” nice one!

(CR seems to not be valuable here, because nothing else should need this, right?)

I'm curious to see if this will break https://www.drupal.org/project/big_pipe_sessionless β†’ πŸ˜‡ Happy to update that. Will know after tomorrow's scheduled CI pipeline πŸ‘

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Just a nit β€” this looks great!

Also, am I missing something, or is this actually saving DB queries?! Nonsensical queries like

SELECT [tag], [invalidations] FROM {cachetags} WHERE [tag] IN ()'

πŸ™ˆ

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

@catch See my comment on the MR β€” I'd be fine with removing this UI, I just figured we're trying to minimize change for site builders during the D10 cycle too. Happy to do so though β€” it'd mean a more consistent UX anyway, so I sort of see the appeal: consistent UX and less code complexity!

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

POST is working, with only 2 @todos remaining.

Next up: PATCH support. Then fix one of the @todos β€” the other one I just proposed a solution for in a comment, but requires an API addition in ConfigManager AFAICT. Marking in case somebody has an idea/insight for that one 🀞

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

The code looks fine (πŸ˜‡), but the comment doesn't quite make sense to me: the comment is more complicated than the logic here πŸ˜„ It's much simpler: if there's no cache tags, then there is no cache tag to invalidate, hence it's always valid.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Oh, oops! πŸ˜…

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Don't feel strongly about it either way. But was definitely going to create a new issue for ExistsIn anyway πŸ‘

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

That's not yet certain. πŸ˜… Plus, we can continue here regardless of that.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Wim Leers β†’ changed the visibility of the branch 2300677-jsonapi-config-entities to hidden.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Oh and I think we already need a follow-up for the first finding, by @kristiaanvandeneynde in #16: we can optimize \Drupal\Core\Cache\CacheTagsChecksumTrait::isValid() with a trivial early return:

if (empty($tags)) {
  return TRUE;
}
πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

πŸš€ This will be very helpful to improve Drupal's use of cache tags 😊 Thanks for making this happen!

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

That "dear committer" note on the MR updates code introduced by #2870878: Add config validation for UUIDs β†’ 6.5 years ago that is now obsolete πŸ₯³

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Aha, that passed β€” so AjaxTest must be prone to random failures…

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Down to a single failure, for AjaxTest::testAjaxFocus(). I'll let that one slide for now and move on to the next system.* simple config πŸ‘

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

#66: it's deprecated, not removed. See #53 for why that is not necessary.

#67: We're more pragmatic than that at this point πŸ˜„ It's been a long time since you touched this, plus I reviewed YOUR changes, so if you can review mine and @dimitriskr's , then … there's no reason you cannot RTBC this! As @alexpott likes to say (paraphrasing)
: "why would you not be allowed to RTBC it if you happen to have the expertise to thoroughly review this particular area?" 😊

… and in that same vein: I've re-reviewed everything and only made trivial tests-only changes to the CKEditor 5 tests, to ensure that @group legacy only appears in the sole legacy test.

Let's get this done! πŸ˜„

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Thank you, @longwave! That's a relief πŸ˜„ I'll push that forward too then πŸ‘

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

This is now looking very close! πŸ˜„

Several follow-ups have been identified β€” let's get those created and let's make this MR point to them. 50% of the remaining work is creating those follow-ups, 50% are for small remarks.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

it costs us very little to take advantage of Composer here,

… this got me thinking … could we do something similar to the way that Drupal core has some "virtual packages" (IDK the correct terminology) to put everything in core/lib/Drupal/Component in its own Composer package?

i.e. if we could put every core/recipes/* in its own composer package, then suddenly everything is perfectly consistent, because then it'd be just like recipes/*: they all are Composer packages for Drupal recipes.

I have no idea how feasible this is though πŸ˜…

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Very close! πŸ‘ I see at least one missing test, one unnecessary method and one accidental public API addition, so for that.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I am not sure if configfield.storage.node.meta_tagswould already exists or need to be created when config action is applied. I think it will be present.

Based on reading the issue summary, I would also assume that field.storage.<entity type>.<something> already exists prior to applying the config action.

But it should be possible for the same recipe to declare that to be installed.

From the config file I need to get entity type which will be used to get the related bundles. From bundles lets say article and page, I need to create field.field.node.article.meta_tags and field.field.node.page.meta_tags. Is this assumption correct?

Based on the IS: yes. πŸ‘

What if one of the field already exists?

Great question! Again based on the IS (and the parent issue), I'd think that is fine. The purpose is to have this field instantiated on all bundles of a fieldable entity type. If some already exist, that's fine.

But that does suggest that createFieldForEveryBundleIfNotExists would be a better name. Or … perhaps it's better still to accept an additional parameter? So that'd become:

field.storage.node.meta_tags:
  instantiateOnAllBundles:
    label: Meta tags
    description: 'Adjust your meta tags.'
    failIfExists: false

We'll need a detailed rationale for either approach, but either way this can continue for now β€” let's allow an extra parameter for this for now and then we can still remove it if that is what we decide.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Legitimate failures:

  • MigrateContactSettingsTest (for both D6 & D7)
  • ContactStorageTest
  • ContactSitewideTest

This is likely because default_form no longer is allowed to be set to '', only to null. That means this needs an update path similar to πŸ“Œ [PP-1] Make NodeType config entities fully validatable Needs review .

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Wim Leers β†’ created an issue.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

If this also would add #config_target support β†’ , then atypical hybrids (simple config being updated as part of a config entity form) such as #3422872-4: Add validation constraints to contact.settings β†’ would've been able to use #config_target too.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Ah, interesting β€” I worked to convert the weird integration of editing the contact.settings simple config from within the ContactFormEditForm config entity form to use #config_target (see https://www.drupal.org/node/3373502 β†’ ), but didn't realize that wouldn't work, because #config_target is only available on ConfigFormBase subclasses. So … there's nothing else to do here!

Attached is the patch containing what I did before I realized it cannot work. Only if ✨ [PP-2] Introduce ConfigEntityForm to standardize use of validation constraints for config entities Active adds #config_target support to config entity forms would this be possible to implement.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

This needed ✨ Follow-up for #3324150: allow specifying a prefix for ConfigExistsConstraint, to enable using it for references to config entities Needs review .

But sadly once again, just like both book.settings (see #3422862-3: Add validation constraints to book.settings β†’ ) and filter.settings before it (see πŸ“Œ Add validation constraints to filter.settings Fixed and #1932544: Remove all traces of fallback format concept from the (admin) UI β†’ ), this simple config violates the basic rule of simple config: that it cannot depend on other config entities. 😬 Fixing that is out of scope here.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Thanks! Alphabetically next unvalidatable simple config with MR to make it fully validatable: πŸ“Œ Add validation constraints to book.settings Needs review and πŸ“Œ Add validation constraints to contact.settings Active . Also semi-automated the updating of the plan issue: #2952037-24: [meta] Add constraints to all simple configuration β†’ .

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Oops, that was done against πŸ“Œ Add validation constraints to book.settings Needs review πŸ˜…

Rather than listing explicit issues in the summary, please see the child issues in the sidebar β€” that makes it easier to keep this up-to-date.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Had some fun with drush and the config.typed service to semi-automate keeping this plan issue up-to-date 😊

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

The ExistsIn constraint I wrote for this MR is already useful in another: see #3422862-4: Add validation constraints to book.settings β†’ .

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

This MR copied the ExistsIn constraint that I wrote yesterday for πŸ“Œ Add config validation for workflow entities. Active . That should have its own issue, but that doesn't mean this cannot already be reviewed.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Wim Leers β†’ created an issue.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Just like filter.settings before it (see πŸ“Œ Add validation constraints to filter.settings Fixed and #1932544: Remove all traces of fallback format concept from the (admin) UI β†’ ), this simple config violates the basic rule of simple config: that it cannot depend on other config entities. 😬 Fixing that is out of scope here.

But we can proceed here, thanks to \Drupal\Core\Config\Schema\SchemaCheckTrait::$ignoredPropertyPaths πŸ‘

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Wim Leers β†’ created an issue.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

@longwave Implemented what I proposed in #9 (but more explicitly/clearly) to address your concerns in #8: https://git.drupalcode.org/project/drupal/-/merge_requests/6700/diffs?co...

So now it should not even be necessary anymore to "just deal with it whenever that [Drush not compatible with next major Drupal version] happens".

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

The one issue I see is that when we switch to main or open a new branch for core development, what happens when composer require drush/drush drupal/config_inspector starts failing because there is no compatible package yet?

  1. config_inspector is already declaring 11.x compatibility and is explicitly testing daily with OPT_IN_TEST_NEXT_MAJOR.
  2. Drush is a different matter indeed.

But, perhaps we just deal with that when it happens by altering the job?

+1 to this.

How about this for a pragmatic balance:

  1. composer require … today exits with code 1 upon failure β€” we can append || exit 99
  2. … then we could use allow_failure:exit_codes (docs) to specifically allow failure only for 99, but not for anything else

Tada, graceful degradation! πŸ˜„

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Yay!

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I started working on ✨ [PP-2] POST/PATCH config entities via REST for config entity types that support validation Needs work and encountered this. Adding one more tag to improve discoverability.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Wim Leers β†’ changed the visibility of the branch 11.x to hidden.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

@smustgrave Yes, they still fail because core is still calling these APIs. And it has to, to avoid breaking BC. Which is what @lauriii indicated in his review on the MR.

it could be tricky since I'm not sure there's a good way for us to retrieve the currently registered event listeners.

Indeed. There is no way AFAICT.

But there is one thing we can do, which I'd like to get @lauriii's thoughts on:

  1. editor.admin.js is loaded automatically for FilterFormatEditForm thanks to editor_form_filter_format_form_alter() doing
        '#attached' => [
          'library' => [
            'editor/drupal.editor.admin',
          ],
        ],
    
  2. but it's already the responsibility of each text editor that uses these APIs to depend on the editor/drupal.editor.admin asset library (otherwise it's possible it be loaded too late!) β€” the CKEditor 4 module does that too for example: https://git.drupalcode.org/project/ckeditor/-/blob/1.0.1/ckeditor.librar...
  3. therefore the solution is fairly simple: remove the auto-loading in #1, and instead rely on asset library dependencies, which it should've been doing all along

(AFAICT the reason the auto-loading was still present: #1894644: Unidirectional editor configuration -> filter settings syncing β†’ introduced that asset library and predates strong use of dependencies, which happened a year later, in #1996238: Replace hook_library_info() by *.libraries.yml file β†’ .)

Implemented that.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

#12: let match() grow on you 😊 It had to grow on me too. The #1 benefit of match() is not brevity, but the language-level help to ensure all cases are covered β€” see UnhandledMatchError.

#14: read https://stitcher.io/blog/php-8-match-or-switch β€” I'd argue the opposite is true: it's switch() that contains more magic than match() πŸ˜…

Keeping at for implementing match() since both @catch and @kristiaanvandeneynde are on board now 😊

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

This would also help make the Recipes initiative more likely to succeed, because more validatable config leads to more reliable recipes, which should lead to a stronger Recipes ecosystem.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

To my own surprise, this pretty much worked on the first try 🀯

(Then again, I did already make a superset of all this work in config_inspector in πŸ“Œ Automated report on core config validatability Needs review , and that was much harder. So all the necessary pieces already existed.)

Impact

This would make the impact of πŸ“Œ Allow vocabularies to be validated via the API, not just during form submissions Active , πŸ“Œ Add validation constraints to announcements_feed.settings RTBC and ✨ New config schema data type: bytes Needs review (the three currently RTBC "config schema + validation" issues, one targeting all config entities of a certain type, one targeting a simple config object, one targeting a single config schema type) much easier to understand.

Reviews/likely concerns

I think there are a few obvious things to disagree with here:

  1. πŸ€” Is it okay for this job to install drush/drush? I could work around this, although there's no trivial way to install modules from the command line then.
  2. πŸ€” Is it okay for this job to install drupal/config_inspector? I could move the drush config:inspect --statistics logic into Drupal core, but that seems far worse, because it increases the maintenance burden of Drupal core?

But … the complexity here is entirely isolated to this single new core CI job. At any point in time can we easily remove this job. In fact, once the linked meta/plan issues are complete, I'd vote to remove this CI job. It's only a tool to help core's velocity and improve both contributor and committer DX.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

The test failures seem to have revealed a long-standing bug in how %parent.%key works for a nested sequence item 😬 We'll need to fix that first.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Broader than this issue, should perhaps discuss in a follow-up

+1 for follow-up to maintain consistency. Tagged for that.

Should we switch to enum? -> Fine by me, but how do we use match() then?

Posted suggestion (https://git.drupalcode.org/project/drupal/-/merge_requests/6643/diffs#no...), proof that it works: https://3v4l.org/VsQ3n

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I got πŸ“Œ Add config validation for workflow entities. Active started too β€” now 100% of the issues listed in the issue summary have an in-progress MR, some RTBC, some nearly, some with still a long way to go.

Progress! 😊

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

This MR gets us to 100% validatability, but at the last moment I discovered there was a whole new edge case: predefined_states_workflow_test_type β€” where possible states are NOT defined in the config 😬

Anyway, this should still serve as a good starting point πŸ‘

P.S.: this might be blocked on πŸ› Sequences of mappings cannot merge keys from a subsequent definition of mapping keys Needs work ?

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Encountered this while working on πŸ“Œ Add config validation for workflow entities. Active .

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I'll get this going…

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

See the parent issue β€” several config entity types in Drupal core are now fully validatable! πŸš€

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Agreed with #6, and curious about the responses to #6 😊

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

A few questions + nits πŸ˜‡ But … I'm definitely intrigued! 🀩

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Note that πŸ“Œ Deprecate vocabulary weight property Needs work is marking this fully validatable, and is adding the following changes wrt weight's config schema:

     weight:
+      # @todo Deprecate in https://www.drupal.org/project/drupal/issues/3008064
       type: integer
       label: 'Weight'
+      # A weight can be any integer, positive or negative.
+      constraints:
+        NotNull: []

πŸ‘† That amounts to no changes at all: it just means that any integer is accepted. It also means that it should remain trivial for this issue to deprecate weight πŸ‘

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

That's really helpful, @Berdir β€” and I'd forgotten about the "permissions local task" pattern! πŸ‘

You've convinced me that this issue is indeed 100% independent of any UI impact. Which means it's definitely in an RTBC'able state πŸ₯³

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Moving this to needs work for the outstanding feedback on the MR.

Addressed! πŸ“

I'm also wondering if the changes that are outside Editor module could be done in their own issue before this one?

There's a few bits we can extract:

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Wim Leers β†’ created an issue.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

The code I just pushed was previously written for ✨ [PP-1] Mark parts of CKEditor 5 and Editor config schema as fully validatable Postponed . Crediting @phenaproxima for his review of that code.

(This now blocks ✨ [PP-1] Mark parts of CKEditor 5 and Editor config schema as fully validatable Postponed .)

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

This at minimum soft-blocks πŸ“Œ [PP-2] Make FilterFormat config entities fully validatable Postponed and potentially hard-blocks it.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Is this still relevant? πŸ€”

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Wim Leers β†’ changed the visibility of the branch 11.x to hidden.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Subsequently add a deprecation notice in FilterFormat::get() and FilterFormat::set() for the roles property.

This part is missing. But … I'd argue it's not necessary, since FilterFormat::postSave() will still trigger a deprecation notice. It might be a slightly nicer/clearer way for contributed/custom module maintainers to be notified though?

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Done.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

#59:

  1. πŸ‘ Thanks for confirming! Updated issue summary.
  2. We should not modify the UI with warnings about what will change β€” that'd certainly cause a usability regression.

    On second thought … I don't think we need special consideration for this β€” literally EVERYTHING ELSE in Drupal requires going to the "permission" UI. This is the only inconsistency left AFAIK.

That means this now is truly RTBC-worthy! 😊

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

#143: that should not be possible 😬

So I stepped through it with a debugger. And sure enough, the cache tags for

entity_view:node:1:full:[languages:language_interface]=en:[theme]=stark:[timezone]=Australia/Sydney:[url.site]=http://core.test:[user.permissions]=391b575d7fe702dd6349da2ae38be4f9b2d581e0906be36e501d3d53a5ed6145

were:
node:1 node_view rendered user:2 user_view

… but \Drupal\filter\Element\ProcessedText::preRenderText() should have bubbled the cache tags:

    // Set the updated bubbleable rendering metadata and the text format's
    // cache tag.
    $metadata->applyTo($element);
    $element['#cache']['tags'] = Cache::mergeTags($element['#cache']['tags'], $format->getCacheTags());

So is there some special handling for disabled text formats that explains this? YES THERE IS! In that same method:

    // If the requested text format doesn't exist or its disabled, the text
    // cannot be filtered.
    if (!$format || !$format->status()) {
      $message = !$format ? 'Missing text format: %format.' : 'Disabled text format: %format.';
      static::logger('filter')->alert($message, ['%format' => $format_id]);
      $element['#markup'] = '';
      return $element;
    }

πŸ‘† This should do the same cache tag merging logic if ($format !== NULL).

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Drupal 11 is getting close, so … let's finally get this done πŸ™ˆ

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Actually … we already have an issue for that: #2567801: Deprecate core/modules/editor/js/editor.admin.js JS APIs in Drupal 10, for removal in Drupal 11 β†’ .

Restoring issue metadata and just not fixing this seems the best solution then? πŸ˜‡

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

#3151100: Replace use of whitelist/blacklist in CkEditor and Editor modules β†’ is now a contrib-only issue, since Drupal 10 no longer includes CKEditor 4.

Furthermore, editor.admin.js contains functionality used only by CKEditor 4. CKEditor 5 does not use this, because it was too brittle β€” countless bugs were related to this JS (written by yours truly πŸ™ˆ).

So repurposing this issue:

  • in Drupal 11 we should remove this file
  • which means that in Drupal 10 we should remove it πŸ‘ˆ rescoping the issue to that
πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

AFAICT this should also change

# Filter with module and status.
filter:
  type: mapping
  label: 'Filter'
  mapping:
    id:
      type: string
      label: 'ID'
      constraints:
        PluginExists:
          manager: plugin.manager.filter
          interface: 'Drupal\filter\Plugin\FilterInterface'
    provider:
      type: string
      label: 'Provider'
    status:
      type: boolean
      label: 'Status'
    weight:
      type: integer
      label: 'Weight'
    settings:
      type: filter_settings.[%parent.id]

to:

filter:
  type: mapping
  label: 'Filter'
  mapping:
    id:
      type: string
      label: 'ID'
      constraints:
        PluginExists:
          manager: plugin.manager.filter
          interface: 'Drupal\filter\Plugin\FilterInterface'
    provider:
      type: string
      label: 'Provider'
    weight:
      type: integer
      label: 'Weight'
    settings:
      type: filter_settings.[%parent.id]

… because all entries in filter.format.*:filters will have status: true.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Discovered one more blocker.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

This will help make πŸ“Œ [PP-2] Make FilterFormat config entities fully validatable Postponed simpler.

Production build https://api.contrib.social 0.61.6-2-g546bc20