Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#19 | protected-2821272-19.patch | 593 bytes | martin107 |
Comments
Comment #2
martin107 CreditAttribution: martin107 as a volunteer commentedTo 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.
Comment #5
joachim CreditAttribution: joachim as a volunteer commentedSo does config entity schema get inherited too? I didn't know that!
Comment #6
martin107 CreditAttribution: martin107 as a volunteer commentedJust 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
previously if the value of $flag::id was "apple" then the string "flag apple()" was fed into the function user_roles... brackets and all.
Comment #9
joachim CreditAttribution: joachim as a volunteer commented> 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.
Comment #10
joachim CreditAttribution: joachim as a volunteer commentedAlso, 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!
Comment #11
martin107 CreditAttribution: martin107 as a volunteer commentedIn 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?
Comment #12
martin107 CreditAttribution: martin107 as a volunteer commentedFrom 5
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
looking the the properties section in the documentation at the "type" key
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.
Comment #15
BerdirConfigEntityBase doesn't actually define the label property, just the schema. Don't ask me why.
Comment #16
martin107 CreditAttribution: martin107 as a volunteer commentedReturning 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?
Comment #17
BerdirThat 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.
Comment #18
martin107 CreditAttribution: martin107 as a volunteer commentedFair 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.
Comment #19
martin107 CreditAttribution: martin107 as a volunteer commentedSorry for my roundabout ways - I have cutback the issue summary to reflect the current state.
Comment #21
socketwench CreditAttribution: socketwench commentedThanks everyone!