Problem/Motivation

I am marking this as a bug in the documentation as without this the Id documentation for the CKEditorPlugin is incomplete and redundant.

Currently the only doc string for the core/modules/ckeditor/src/Annotation/CKEditorPlugin.php CKEditorPlugin.id is "The plugin ID."

This has two problems.

  1. It uses "ID" when it should be "Id"
  2. It is completely useless and redundant because the attribute name is "id" so it is obvious that we are defining an Id

Proposed resolution

There are actually some requirements for this attribute so we should have some documentation that points that out. I opened #3000962: [upstream] Improve JS error when a @CKEditorPlugin PHP plugin ID doesn't match the JS CKEditor plugin name to attempt to improve the errors that happen if this attribute isn't set correctly, but it also should be mentioned in the docs what those requirements are. As far as I know the only requirement is that the Id matches name declared to CKEditor in the JS.

Remaining tasks

Write doc patch.

User interface changes

none

API changes

none

Data model changes

none

CommentFileSizeAuthor
#3 3001029-3.patch725 byteswim leers
#2 improve-ckeditor-plugin-annotation-doc-3001029-2.patch792 bytesfrob

Comments

frob created an issue. See original summary.

frob’s picture

frob
Primary language English
commented
StatusFileSize
new improve-ckeditor-plugin-annotation-doc-3001029-2.patch792 bytes

Here is a first pass at the patch to improve the documentation.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Improve the documentation of the CKEditor Plugin Annotation » Improve the documentation of the @CKEditorPlugin plugin type annotation
Category: Bug report » Task
Status: Active » Needs review
StatusFileSize
new 3001029-3.patch725 bytes
  1. It uses "ID" when it should be "Id"
  2. It is completely useless and redundant because the attribute name is "id" so it is obvious that we are defining an Id

We always write ID in text/comments. We only write Id in either PHP class names, PHP class method names or PHP class properties. I completely agree it's redundant and obvious but … that's what the documentation maintainers have decided, and so you'll see this for every plugin annotation class:

  • \Drupal\quickedit\Annotation\InPlaceEditor
  • \Drupal\rest\Annotation\RestResource
  • \Drupal\Core\Layout\Annotation\Layout
  • \Drupal\views\Annotation\ViewsRow
  • …

There are actually some requirements for this attribute so we should have some documentation that points that out.

+1! Your patch has some language and formatting issues. What do you think about the attached patch?

frob’s picture

frob
Primary language English
commented

Sorry about the whitespace errors, I guess I need to finish setting up vscode to delete that when I save.

I completely agree it's redundant and obvious but … that's what the documentation maintainers have decided, and so you'll see this for every plugin annotation class.

I guess if there is nothing else there it is better than nothing, but in the case where the ID is actually used for something more than identification we should flesh those out a bit more. I have no idea which plugin annotation classes would need that and it seems like a pretty daunting task. But at least this one is getting some attention.

I don't think its as informative or verbose as what I wrote, but it still gets the job done. Mine is a bit too wordy.

I would mark this as RTBC, but since I wrote the initial patch I am not sure I should.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I don't think its as informative or verbose as what I wrote, but it still gets the job done. Mine is a bit too wordy.

I agree yours was a bit too wordy :) Didn't want to spend too many words on that wordiness, hence I just made a counterproposal. Hope you don't mind!

I would mark this as RTBC, but since I wrote the initial patch I am not sure I should.

You reported the confusion initially, so if you think that my alternative proposed documentation still addresses the confusion, then I think you're totally allowed to RTBC this :)

frob’s picture

frob
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Done! RTBCed then :)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

🎉

  • lauriii committed 6eb60a3 on 8.7.x 
    Issue #3001029 by frob, Wim Leers: Improve the documentation of the @...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Fixed

Committed d207c7d and pushed to 8.7.x. Also cherry-picked to 8.6.x as this only touched documentation. Thanks!

  • lauriii committed 47b271a on 8.6.x 
    Issue #3001029 by frob, Wim Leers: Improve the documentation of the @...

Status: Fixed » Closed (fixed)

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