Updated: Comment #0

Problem/Motivation

Over at #2109769: CKEditor plugin cache entries should have cache tags, I updated one plugin manager's setCacheBackend() call to also assign a cache tag. I believed this was necessary to be able to clear all cached plugin metadata that gets stored in the DB.

I was very, very wrong.

We already have clearCachedDefinitions() for that.

The thing is: I've used clearCachedDefinitions() many, many times. Mostly in tests. I just always thought it was for static cache clearing only. Apparently it's also for persistent cache clearing.

Proposed resolution

CachedDiscoveryInterface::clearCachedDefinitions() already states:

   * Clears static and persistent plugin definition caches.

So it's already documented. But after having talked to berdir, we think it could be more explicit. This is an attempt of doing so.

Remaining tasks

None.

User interface changes

None.

API changes

None.

#2109769: CKEditor plugin cache entries should have cache tags

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Status: Active » Needs review
FileSize
818 bytes

This is the best I could think of. I'd understand it if people think this is either slightly silly or completely ridiculous. All I have to say in my defense is: I think this might help some users.

After having talked to berdir and him suggesting I improve the docs, this is the best I could think of. Maybe some of you have a better idea :)

Berdir’s picture

Works for me.

I guess what I meant is to document setCacheBackend() better, to state what cache tags should be used for and what not.

Wim Leers’s picture

Title: Attempt to improve CachedDiscoveryInterface::clearCachedDefinitions() docs » Attempt to improve CachedDiscoveryInterface::clearCachedDefinitions() and DefaultPluginManager::setCacheBackend() docs
Status: Needs review » Needs work

#2: you're right, I should do that too.

Wim Leers’s picture

Issue summary: View changes

Updated issue summary.

Wim Leers’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +docs
FileSize
1.83 KB
1.04 KB

What about this?

Berdir’s picture

Component: plugin system » documentation

Content works for me, @jhodgdon might have some feedback on the form ;)

jhodgdon’s picture

Status: Needs review » Needs work
Issue tags: -docs

I have two nitpick comments:

a)

+   * Don't resort to calling \Drupal::cache()->delete() and friends to make
+   * Drupal detect new or updated plugin definitions, always use this method on
+   * the appropriate plugin type's plugin manager!

This is a comma splice. Should be a ; or a . between these two complete sentences.

    *   (optional) When providing a list of cache tags, the cached definitions
    *   are tagged and are used to clear the cache.
+   *   Note that this should be used with care! For clearing plugin cache
+   *   entries, use clearCachedDefinitions(). Only use this when plugin cache
+   *   entries should be cleared along with associated cache entries.

Please wrap the new text starting at the end of the previous line. Also I thought the word "this" might be slightly ambiguous in the last sentence -- could it say "Only use this parameter..."? ... Actually, this whole @param doc doesn't make a bit of sense to me, like "the cached definitions are tagged and are used to clear the cache."? that says the definitions are used to clear the cache, but I think it is the tags that are used maybe? This whole @param could be rewritten to be clearer. Please. :)

Also, please don't add a tag 'docs' to an issue that is in the documentation component. It just encourages people to add random issue tags. At least in Core, we try to have useful and meaningful tags (and we even have tag guidelines, which are linked below the tag field). Thanks!

Wim Leers’s picture

Component: documentation » plugin system
Status: Needs work » Needs review
Issue tags: +docs
Related issues: +#2109769: CKEditor plugin cache entries should have cache tags
FileSize
2.23 KB
2.52 KB
  1. Fixed.
  2. Fix attempted :) Please review!

Regarding the "docs" tag: I added that tag when this wasn't in the "Documentation" component. And, honestly, I think it's inappropriate to move anything that is not related to the documentation system itself into the "Documentation" component: how are people then supposed to find out what part of the system it's related to? This is a "docs" thing about the "plugin system".

jhodgdon’s picture

Component: plugin system » documentation
Status: Needs review » Reviewed & tested by the community

The reason for moving things into the Documentation component is primarily so that I will see them, because I watch that component and will review and commit issues only from there. So usually what happens is what happened on this issue: the technical and module-maintainer issues are resolved first and the issue is in that component, and then it is moved to Documentation for final wording, grammar, and standards review/commit by myself and/or others who watch Documentation issues and review them.

Also if there are issues in Documentation that have technical questions that are not obvious, I generally move them into that other component and ask for the maintainers of that component to answer them; however, once they are resolved, I will not see the issue in the "OK for Jennifer to commit" queue if it is not in Documentation, because my mandate is only to commit API docs patches (except for emergency fixes of head and rare cases like that).

Anyway, I love your rewrite of that 2nd topic. It is very clear now. Thanks! Setting to RTBC pending perhaps anyone else following this issue having a different opinion.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Thanks again! Committed to 8.x.

Wim Leers’s picture

Issue tags: -sprint
jhodgdon’s picture

It's not that it *wouldn't* suffice... But why "docs" in particular? Just start typing "doc" in the Tags box. There are lots of tags:
docs
documentation
d7docs
d8docs
documentation bug
etc.
See, the problem with tags is that they are free form and people are (despite the guidelines links below the tags field, which no one reads) always inventing new ones.

So, we have instead standardized in Drupal Core on using the "documentation" component. There are quite a few people (including myself) that find it quite useful to be able to have one central place to look for API docs issues. The system is working well... why change it?

Wim Leers’s picture

Why change it? Because it'd be better.

Because "Documentation" is not a subsystem/module of its own. Documentation must be written and exists for *every* single subsystem/module. Several times already, I've found out much too late that there was an issue related to one of the core modules I'm a maintainer for in the "Documentation" component, which I would've found much sooner if it existed in the module's component.

If we'd apply the reasoning that's being applied here to everything, then e.g. the "CSS" and "JavaScript" components should contain hundreds of additional issues. Imagine how dysfunctional it would be to find a module-specific CSS problem in the "CSS" component — how would component maintainers possibly be expected to find those?

So that's why I'm arguing the "CSS", "JavaScript" and "Documentation" components should only contain "meta" and "standards" issues. Anything that's specific to a subsystem/module should go into that component.

Note that this is already how nod_ (the JavaScript maintainer) tracks JavaScript issues across all of Drupal core: https://drupal.org/project/issues/drupal/search?issue_tags=JavaScript !

I do understand that the current system works well for you. But it inherently works poorly for component maintainers.

xjm’s picture

@Wim Leers, I think it might make sense to open a separate policy issue for this to discuss it so we don't hijack this issue? :)

jhodgdon’s picture

I was just going to say the same thing. Definitely if you want to open up a discussion it needs to be a separate issue so that others doing core dev can respond properly.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.