Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Part of meta-issue #2016679: Expand Entity Type interfaces to provide methods, protect the properties.
See the detailed explanations there and look at the issues that already have patches or were commited.
Add get*, set* and additional methods as it makes sense to replace the public properties (e.g. isSomething() and something())
Comment | File | Size | Author |
---|---|---|---|
#49 | 2030655.48.patch | 4.8 KB | wwhurley |
#47 | 2030655.47.patch | 4.91 KB | alexpott |
#47 | 46-47-interdiff.txt | 1.2 KB | alexpott |
#23 | expand-role-with-methods-reroll-2030655-22.patch | 12.54 KB | jamesquinton |
#21 | expand-role-with-methods-2030655-21.patch | 22.1 KB | Alumei |
Comments
Comment #1
undertext CreditAttribution: undertext commentedComment #2
steeloctopus CreditAttribution: steeloctopus commentedComment #3
steeloctopus CreditAttribution: steeloctopus commentedComment #4
steeloctopus CreditAttribution: steeloctopus commentedI believe I should be able to resolve this issue soon, but I'm very new to the drupal community so I will have a chat will drupal core mentoring team to find out more before I can resolve this problem
Comment #5
artem_sylchukHm, I started work on this month ago and created patch, but haven't posted here. I think it may be helpfull. I'll try to continue work on this.
Comment #8
daffie CreditAttribution: daffie commented@james_kerrigan: I have reviewed your patch and it looks promising.
There are a couple of points I would like to make.
1. In the file: core/modules/user/lib/Drupal/user/RoleInterface.php
I have changed the documentation a bit to bring it in line with the drupal standard
2. In the file: core/modules/user/lib/Drupal/user/Entity/Role.php
My suggestion for the function definitions would be:
Some of the public variables have to change to protected.
And on line 128 you missed !isset($this->weight) to !$this->getWeight()
Comment #9
rdatar CreditAttribution: rdatar commentedAdded call to getWeight() line 128.
Comment #10
rdatar CreditAttribution: rdatar commentedComment #11
Sharique CreditAttribution: Sharique commented@daffie documentation seems correct to me, same as in other parts of D8.
Comment #13
InternetDevels CreditAttribution: InternetDevels commentedI've got some problems with changing class properties ro protected.
First of all fails core/modules/user/lib/Drupal/user/Tests/UserRoleAdminTest.php 114 here:
Looks like it is something with role save. I'll try to continue work but maybe somebody else find a solution for this faster.
Comment #15
daffie CreditAttribution: daffie commentedComment #17
daffie CreditAttribution: daffie commentedComment #18
pcambraJust a plain reroll of this.
Comment #19
Alumei CreditAttribution: Alumei commentedAnd again a plain re-roll.
Comment #20
dawehnerIf you change this you can also use $account->getUsername();
Maybe it would be a little bit helpful to explain what a weight of a role means.
Comment #21
Alumei CreditAttribution: Alumei commentedContains fix of #20.1 but for #20.2 I'm at loss. I actually do not know about the weight part of user roles.
Comment #22
jamesquinton CreditAttribution: jamesquinton commentedI'm going to reroll the patch.
Comment #23
jamesquinton CreditAttribution: jamesquinton commentedReroll for patch in #21
Comment #24
jamesquinton CreditAttribution: jamesquinton commentedComment #25
dawehnerAll those check_plain => String::checkPlain changes are certainly fine, thought they seem to be a little bit out of scope.
You could simply go with return (bool) $this->get('weight')/$this->getWeight()
We do use now just @return $this and no doc line below
Comment #26
cosmicdreams CreditAttribution: cosmicdreams commentedComment #27
ericduran CreditAttribution: ericduran commentedComment #28
justafishI rerolled and removed the hasWeight function as I don't think it makes sense - something could have a weight of 0 and this method would return false. I also changed the documentation as per #25. Everything else looks good to me.
Comment #30
justafishOops
Comment #31
justafishComment #32
justafishComment #34
justafishComment #35
justafishComment #36
justafishComment #37
justafishsame patch, testbot isn't picking it up from #32 :|
Comment #38
justafishCoding standards and spelling correction
Comment #39
mtiftThese should both be@return self
These should both be
@return $this
: https://drupal.org/node/1354#typesComment #40
justafishI've removed the conversion of check_plain as it's already being done in #2089331: [meta] Replace calls to check_plain() with Drupal\Component\Utility\String::checkPlain()
New patch also switches the property visibility to protected.
Comment #41
justafishComment #42
justafishComment #43
justafishComment #44
justafishComment #45
justafishComment #46
justafishComment #47
alexpottNo longer "needs followup" the check_plain has been fixed in #2089331: [meta] Replace calls to check_plain() with Drupal\Component\Utility\String::checkPlain().
Protected all the public properties and removed toArray since #2256679: Use config schema to determine which config entity properties to export regardless of whether they are public or protected has gone in.
Comment #49
wwhurley CreditAttribution: wwhurley commentedPatch applies mostly cleanly, just a little fuzz on a couple of hunks. Attached is a re-roll moving the start lines down a bit. Tested manually manipulating user roles and the patched version works as intended.
Comment #51
catchCommitted/pushed to 8.x, thanks!