I have just noticed a systematic error running through core ... and I want to also fix this on the contrib modules .. I care about..

If one defines a id variable on a class that extends Plugin ... it inadvertently converts a read-only variable into a read-write variable ... well read, write ( accept and ignore ) variable..

I have a issue in core to remove the 34 bugs.... #2917345: Parallel definition of ID is confusing

Also core sometimes needlessly redefines things like label and title.... and I want to remove that from flag also
#2917213: FieldType: Remove redefinition of label and description

In truth these are all minor ... I think, so I am just going to munge them into one issue here.

CommentFileSizeAuthor
#2 2917359-2.patch1.54 KBmartin107
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

martin107 created an issue. See original summary.

martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented
Assigned: martin107 » Unassigned
Status: Active » Needs review
FileSize
2917359-2.patch1.54 KB

Now I have an issue number here is the patch.

martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented
Title: Make Plugin id's read-only, don't duplicate label » Make Plugin id read-only, don't duplicate label
socketwench’s picture

socketwench CreditAttribution: socketwench at TEN7 commented
Status: Needs review » Postponed

It makes sense, but I'm wondering why no one in core didn't notice this yet. Maybe there's a reason no one bothered to document? I'm going to mark this as postponed for now until we see what core does.

joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented 
+++ b/src/Entity/Flag.php
@@ -68,14 +68,6 @@ use Drupal\flag\FlagInterface;
-
-  /**
-   * The flag ID.
-   *
-   * @var string
-   */
-  protected $id;
-
   /**
    * The entity type this flag works with.
    *
@@ -83,13 +75,6 @@ class Flag extends ConfigEntityBundleBase implements FlagInterface {

@@ -83,13 +75,6 @@ class Flag extends ConfigEntityBundleBase implements FlagInterface {
    */
   protected $entity_type = NULL;
 
-  /**
-   * The flag label.
-   *
-   * @var string
-   */
-  protected $label = '';
-

I don't see a problem with redefining these, as it allows us to give them a docblock description that is more specific to how we use them -- instead of 'the entity ID' and 'the entity label' (which given how many entities we have flying around in this module -- Flag, Flagging, Flaggable -- is a good thing).

And I don't see $id defined in core/lib/Drupal/Component/Annotation/Plugin.php.

  • martin107 authored 84a117d on 8.x-4.x 
    Issue #2917359 by martin107, socketwench, joachim: Make Plugin id read-...
socketwench’s picture

socketwench CreditAttribution: socketwench at TEN7 commented
Status: Postponed » Fixed

Yeah...I'm overthinking. The advantage of the docblock is a good one to have too. Committed!

joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented
Status: Fixed » Active

Could you revert this please? All of those changes are undesirable.

  • 6475d44 committed on 8.x-4.x 
    Revert "Issue #2917359 by martin107, socketwench, joachim: Make Plugin...
socketwench’s picture

socketwench CreditAttribution: socketwench at TEN7 commented

Done.

joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented
Status: Active » Closed (works as designed)

Thanks!