Flag::uuid is a duplicate of ConfigEntityBasE::uuid

There is a minor kerfuffle that our redefinition introduces - Flag::uuid is directly accessible!

While reviewing the visibility of Flag properties - it was discovered that Flag::weight should also not be public.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

martin107 created an issue. See original summary.

martin107’s picture

Assigned: martin107 » Unassigned
Status: Active » Needs review
FileSize
1.35 KB

To my mind the property Flag:id is a curious fish.

Looking at the chain

Flag-->ConfigEntityBundleBase-->ConfigEntityBase-->Entity

The getter is defined at the base level Entity::id() --- but the property must defined at the top in Flag

This is a code smell and makes me thing we are doing something horribly wrong. - comments welcome.

Anyway I have deleted entries for Flag::uuid and Flag::label() and made Flag::id protected.

Status: Needs review » Needs work

The last submitted patch, 2: id-2821272-2.patch, failed testing.

The last submitted patch, 2: id-2821272-2.patch, failed testing.

joachim’s picture

+++ b/config/schema/flag.schema.yml
@@ -5,12 +5,6 @@ flag.flag.*:
-    uuid:
-      type: string
-      label: 'UUID'
-    label:
-      type: label
-      label: 'Name'

So does config entity schema get inherited too? I didn't know that!

martin107’s picture

So does config entity schema get inherited too?

Just to be clear ... I am learning too ... I will dig into the code .. confirm or disprove that and report back to this issue...

In the meantime, standardization has unearthed a bug.

/src/Plugin/views/relationship/FlagViewsRelationship.php

in the method query()

now that Flag:id is no longer public this showed up in the error logs

-      $flag_roles = user_roles(FALSE, "flag $flag->id()");
+      $flag_roles = user_roles(FALSE, "flag " . $flag->id());

previously if the value of $flag::id was "apple" then the string "flag apple()" was fed into the function user_roles... brackets and all.

Status: Needs review » Needs work

The last submitted patch, 6: id-2821272-5.patch, failed testing.

The last submitted patch, 6: id-2821272-5.patch, failed testing.

joachim’s picture

> now that Flag:id is no longer public this showed up in the error logs

I'll file another issue for that.

#2821351: remove all direct references to $flag->id.

joachim’s picture

Also, were we changing a protected property in a parent class to public? I wouldn't have thought that was even possible!

EDIT: http://stackoverflow.com/questions/16877875/php-change-method-function-v...

Huh. That seems kinda wrong to me!

martin107’s picture

In the article I see private cannot be redefined --- so that is something.... but my perspective is that the whole duck-typing thing is a worse misstep.
but hey PHP is a useful tool and I can't get too grumpy as Java and Rust exist.

There is one more direct call to Flag->id in flag.module... I think that is the last conversion.

by creating #2821351: remove all direct references to $flag->id are you saying that this issue should be postponed until then?

martin107’s picture

From 5

So does config entity schema get inherited too?

the documentation has been refined a lot since I last looked at this topic. It is much better

https://www.drupal.org/docs/8/api/configuration-api/configuration-schema...

From flag.schema.yml

flag.flag.*:
  type: config_entity
  label: 'Flag'
  mapping:

looking the the properties section in the documentation at the "type" key

The type of the value; can either be a base type or a derived type (see examples below).

https://www.drupal.org/docs/8/api/configuration-api/configuration-schema...

so that is where the inheritance from config_entity happens....

Regarding the new patch -- I would rather catch the last remaining instance of direct access to Flag::id here and close
#2821351: remove all direct references to $flag->id but hey I am flexible .. and so will do whatever is needed to solve both issues.

Status: Needs review » Needs work

The last submitted patch, 12: id-2821272-12.patch, failed testing.

The last submitted patch, 12: id-2821272-12.patch, failed testing.

Berdir’s picture

+++ b/src/Entity/Flag.php
@@ -92,13 +83,6 @@ class Flag extends ConfigEntityBundleBase implements FlagInterface {
   /**
-   * The flag label.
-   *
-   * @var string
-   */
-  public $label = '';
-
-  /**

ConfigEntityBase doesn't actually define the label property, just the schema. Don't ask me why.

martin107’s picture

Returning to this aspect of the issue from #2

> Flag-->ConfigEntityBundleBase-->ConfigEntityBase-->Entity

> The getter is defined at the base level Entity::id() --- but the property must defined at the top in Flag

thanks Berdir, .. it looks like the same clunkiness applies to Entity::label() and Flag::$label

Before I created a core issue I went looking for an existing one.

I am linking to what I found... which relates only to id but I think the same goes for label.

That pointed out to me that values for "id" and "label" are coded into the annotation variables - and the proposed solution from there is to get Entity:id() to use getEntityKey() - which requires thinking about the potential performance hit.

That issue stalled in 2014 - does anyone have any preferences?

Berdir’s picture

That issue would IMHO only be relevant if our properties would *not* be called label/id/uuid. They are, so we can rely on the default implementation.

We could create a core issue to explicitly define the properties in ConfigEntityBase as protected, but there is a certain risk that they are differently defined in a subclass, e.g. as private and then you get a parse error.

Anyway, I'd just define them and move on. Even if such a core issue would get in, it wouldn't happen until 8.3, we don't want to rely on that yet.

martin107’s picture

Title: Standardize Flag::id,uuid,label » Flag - protect label, id and uuid

Fair enough, I will express my concerns about id ( and uuid for that matter ) in core issues.

Now that #2821351: remove all direct references to $flag->id is in - much of this patch becomes outdated and MOST of the rest is out of scope.

There are things I still want to do here...

A) Flag:label needs to become protected and the one direct use of label I can see need to be corrected. ( that is a fragment of the patch that is still relevant. )
B) Flag:uuid needs to become protected - Entity::uuid() is a pre-existing getter method.
C) Flag::weight needs to become protected, as Flag::getWeight() is a thing.

martin107’s picture

Title: Flag - protect label, id and uuid » Flag - protect weight and uuid
Issue summary: View changes
Status: Needs work » Needs review
Related issues: +#2821923: Remove direct reference to Flag::label
FileSize
593 bytes

Sorry for my roundabout ways - I have cutback the issue summary to reflect the current state.

  • socketwench committed 5516209 on 8.x-4.x authored by martin107
    Issue #2821272 by martin107: Updated Flag class to protect weight and...
socketwench’s picture

Status: Needs review » Fixed

Thanks everyone!

Status: Fixed » Closed (fixed)

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