NodeType class variables should not be accessed directly. Functions should be use to access the variable. For instance use getDescription() and setDescription($description) for the protected class variable description. For a boolean variable the getter function becomes isVariableName(). In object-oriented programming this is called encapsulation.
Remaining tasks
- Update the class variables and make them protected.
- Create getters and setters for frequently used get and set functionality.
- Update drupal to use the getters and setters instead of accessing variables directly.
- There are no tests required because the added functions are only getters and setters.
For more info over what should be done see the issue summary of #2016679: Expand Entity Type interfaces to provide methods, protect the properties.
Beta phase evaluation
Issue category | Bug because properties should not be public, API methods should not be allowed to be sidestepped. |
---|---|
Issue priority | Major because this meta goes across the entire system. But each child will be a normal bug. |
Prioritized changes | Prioritized since it is a bug and it reduces fragility. |
Disruption | Somewhat disruptive for core as well as contributed and custom modules:
|
But impact will be greater than the disruption, so it is allowed in the beta.
Comment | File | Size | Author |
---|---|---|---|
#83 | seven.pass_.patch | 3.21 KB | larowlan |
#76 | seven.pass_.patch | 1.87 KB | larowlan |
#76 | seven.fail_.patch | 854 bytes | larowlan |
#68 | 2384481-68.patch | 41.12 KB | rpayanm |
#68 | 2384481-interdiff.txt | 609 bytes | rpayanm |
Comments
Comment #1
daffie CreditAttribution: daffie commentedI would like to get this fixed. So I will do a good review for posted patches.
Comment #2
daffie CreditAttribution: daffie commentedComment #3
claudiu.cristeaThis is not really a novice issue because I needed to rename also the
id
andlabel
ofNodeType
. There was a@todo
left there and I felt that this is the right moment for that as is strong related.Here's a first patch but due to the large number of affected files I didn't manage to test it locally. So, I'm expecting errors. Let's see.
Comment #5
daffie CreditAttribution: daffie commented@claudiu.cristea: Please do not change the class variables names. You only have to make them protected. I have looked at your patch and the functions that I would add are getName(), getDescription() and getHelp(). There is already a function id(). In case that you have no setVariableName() function you can use the build in function set('variable_name', $value) or get('variable_name') instead of getVariableName().
Comment #6
rpayanmComment #9
daffie CreditAttribution: daffie commentedThese function are probably not used. Maybe only in testing. You can remove them and use set('name', $value), set('description', $value) and set('help', $value).
The other added functions need to be added to the NodeTypeInterface.
Comment #11
areke CreditAttribution: areke commentedUsed daffie's comments in #9 for this patch.
Comment #13
areke CreditAttribution: areke commentedThere was a typo in the previous patch.
Comment #15
daffie CreditAttribution: daffie commentedThe reason that your patch failed testing is that outside the NodeType class the class variables are no longer accessible. And it will result in an error. Use the created methods to fix this. If you need the set a variable use set('variableName', $value).
You are editing the NodeTypeInterface not the NodeInterface. So do "of the nodetype" and not "of the node".
Comment #16
rpayanmComment #18
rpayanmComment #20
rpayanmComment #22
rpayanmComment #24
rpayanmComment #26
daffie CreditAttribution: daffie commentedUse id() not get('type').
Use label() not get('name').
Comment #27
daffie CreditAttribution: daffie commented@rpayanm: Can you first start at changing the nodetype variables in the file NodeController.php. This will solve a lot of fails.
Comment #28
rpayanmLet's go!
Comment #30
daffie CreditAttribution: daffie commentedFrom 364 fails and 370 exceptions to 149 fails and 55 exceptions. :-)
Can you add an interdiff.txt file. It is easier to follow what you changed.
Comment #31
rpayanmComment #33
rpayanmComment #34
rpayanmComment #36
Mile23Patch doesn't apply.
Comment #37
rpayanmComment #39
rpayanmdamn! missing a close parenthesis ")"
:D
Comment #41
rpayanmtrying again...
Comment #42
rpayanmComment #44
rpayanmComment #45
rpayanmYeah! :D
Comment #46
daffie CreditAttribution: daffie commentedSuper minor documentation change. See added file interdiff.txt.
Comment #47
daffie CreditAttribution: daffie commentedThe patch in comment #44 looks good to me.
I have one little documentation change. See the added interdiff.txt file.
I the patch are two new functions added: getHelp() and getDescription().
Both are also added to the NodeTypeInterface.
The problem described is the issue summary is fixed with the patch.
Despite the minor change to the patch file from me, I am giving the patch a RTBC.
@alexpott: You want to add the least number of new getter and setter functions.
This is fine by me, but this will result in some strange code constructions.
There are no setId() or setLabel() functions. What do you want to do?
Comment #48
rpayanmMissing the dot.
Comment #52
Mile23Patch in #8 no longer applies.
Comment #53
tadityar CreditAttribution: tadityar commentedRe-rolled
Comment #55
tadityar CreditAttribution: tadityar commentedForgot to save the file.. sorry :( Re-rolled again.
Comment #56
daffie CreditAttribution: daffie commented@tadityar: Can you add an interdiff.txt file with your patch. It is for other people very convenient to see what the changes are between your patch and the previous one.
I think something went wrong here. Did you do "git reset --hard" before start working on this patch?
Comment #58
hussainwebI am doing a straight reroll of #48 as there seems to be something else wrong with the other two patches. There was only one conflict in node.module.
@tadityar: Did you make any other changes? You mentioned that these patches were only rerolls.
Comment #59
tadityar CreditAttribution: tadityar commented@daffie, hussainweb: hmm I didn't make any other change.. It had conflict in node.module and I deleted the
and the patch is like that. I also wonder why it has invalid syntax
Comment #60
BerdirYou could replace this with $type->link().
Comment #61
daffie CreditAttribution: daffie commented@berdir: The idea is good, but I see a problem. In the nodetype annotation there are two links: "edit-form" and "delete-form". The same for the node annotation. There is no link to "node.add". Maybe I am missing something. If so please tell me.
Comment #62
daffie CreditAttribution: daffie commentedIt all looks good to me.
There are 2 methods added: getHelp() and getDescription().
Both are added to the interface.
All documentation is in order.
It gets a RTBC for me.
Comment #65
hussainwebRerolling.
Comment #66
daffie CreditAttribution: daffie commentedGood reroll. Back to RTBC.
I was unable to create an interdiff.txt file.
Thanks hussainweb!
Comment #68
rpayanmComment #69
a_thakur CreditAttribution: a_thakur commentedComment #70
daffie CreditAttribution: daffie commentedIt all looks good to me.
The patch fixes the problem as described in the issue summary.
There are two new methods added to the class NodeType: getHelp() and getDescription().
All documentation is in order.
Good reroll! Back to RTBC.
Comment #71
alexpottCommitted ff7473e and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.
Comment #73
LewisNymanSorry gang but it seems that something here has broken HEAD. When I got to /node/add I get the error:
Fatal error: Cannot access protected property Drupal\node\Entity\NodeType::$type in /Library/WebServer/Documents/core/themes/seven/seven.theme on line 76
Comment #74
webchickConfirmed. :(
Comment #75
cilefen CreditAttribution: cilefen commentedDoes it follow that no test in Drupal gets 'node/add'?
Comment #76
larowlanfailing test + fix
Comment #77
larowlanmost likely only in stark
Comment #78
davidhernandezI didn't look at it, but can confirm that #76 made the white screen of death go away.
Comment #79
dawehnerSeems legit.
Comment #80
cilefen CreditAttribution: cilefen commented#76 fixed it for me
Comment #83
larowlanComment #84
dawehnerMore test coverage!
Comment #85
daffie CreditAttribution: daffie commentedSorry about breaking head!
The patch looks good.
The breaking of head is fixed and a test is added.
It gets a RTBC from me.
Comment #86
webchickNo problem, daffie, you're a real core contributor now! :D
Committed and pushed to 8.0.x. Thanks so much for the quick work on this, larowlan!
Comment #88
larowlanComment #90
andypostexamples in should be also updated #2607294: Fix entity.api bundle examples