Filterformat 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 |
---|---|---|---|
#56 | 2384487-56.patch | 701 bytes | daffie |
#55 | 2384487-55.patch | 1.6 KB | daffie |
#47 | 2384487-47.patch | 24.23 KB | daffie |
#44 | 2384487-44.patch | 24.28 KB | 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
daffie CreditAttribution: daffie commentedComment #4
daffie CreditAttribution: daffie commentedThe class variables $format, $name and $weight need to become protected.
Comment #5
Sharique CreditAttribution: Sharique commentedI've also changed the name for variables, which was specified in todo. Do we need setId function? I think we need it.
Comment #7
daffie CreditAttribution: daffie commentedI would not rename the class variables. I know there is a @todo is the code. The problem is that renaming a variable will result in a lot of changes. And I have the idea that that will not be allowed (beta changes).
This function is not necessary. You can use the build in function label().
These function are probably not used. Maybe only in testing. You can remove them and use set('name', $value) and set('weight', $value)
Comment #8
rpayanmComment #10
rpayanmRound 1
Comment #12
daffie CreditAttribution: daffie commentedUse the function id() for this instead of get('format').
Comment #13
rpayanmRound 2
Comment #14
rpayanm@daffie ohhh thank you.
Comment #15
rpayanmComment #25
rpayanmohh god! appear that testbot it's on trouble :(
Comment #27
rpayanmNo rest!
Comment #29
rpayanmComment #31
rpayanmComment #33
rpayanmComment #35
areke CreditAttribution: areke commentedComment #38
daffie CreditAttribution: daffie commentedUse label() instead of get('name'). In the annotation of the FilterFormat class is the variable $name defined as being the label. And the label has its own getter function called label().
After this change it is RTBC for me. Good work ;-)
Comment #39
rpayanmComment #40
daffie CreditAttribution: daffie commentedAll the class variables are protected.
There are already getter functions available, so no need for new ones.
The test-server give it green.
It all looks good to me, so for me it is RTBC.
Comment #41
alexpottPatch needs to be rerolled... it looks good though.
Comment #44
rpayanmRerolled :)
Comment #45
daffie CreditAttribution: daffie commentedThe file core/modules/text/src/Tests/TextFieldTest.php was updated. So a reroll was necessary. Also for me RTBC.
Comment #46
alexpottLet's use label() instead.
Comment #47
daffie CreditAttribution: daffie commentedI thought that the get('name') was replaced by label() in the past. Somehow it got back in. :-(
Comment #48
daffie CreditAttribution: daffie commentedDouble checked it. The file core/modules/text/src/Tests/TextFieldTest.php was updated. So a reroll was necessary. All the get('name') are gone again.
Comment #49
alexpottCommitted 3ec6348 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation for to the issue summary.
Comment #51
Wim LeersThis broke HEAD.
EditorImageDialog
still contains:And because
format
is now a protected property, a fatal error is thrown.I don't know what's preferred: a revert + reroll, or a new issue to fix it.
Comment #52
daffie CreditAttribution: daffie commented@Wim Leers: How much problems is it giving now?
We need to fix this and add a test to make sure it does not happen again.
Comment #53
daffie CreditAttribution: daffie commentedSame problem in core/modules/editor/src/Plugin/InPlaceEditor/Editor.php (line 39).
The bug from commend #51 is in file core/modules/editor/src/Form/EditorImageDialog.php (line 51).
Comment #54
Wim Leersdaffie: it's making it impossible to upload or edit images via CKEditor. So: a big problem.
Comment #55
daffie CreditAttribution: daffie commentedQuick fix without new tests.
Comment #56
daffie CreditAttribution: daffie commentedIt is a bit confusing to me when ->format is from the filter_format entity en when it is the property of a text field. So also a patch to only quick fix the bug reported by Win Leers.
Comment #58
Wim Leers#55 is wrong, #56 is correct. The difference is quite big: just looking for
->format
is not enough. The first hunk in #55 is getting the "format" property on aFilterFormat
entity, the second is getting the "format" property on aTextItem
.But #56 is never going to be committed here. It needs to be put in its own issue. We no longer do "follow-up commits".
That's why I wrote:
Comment #59
daffie CreditAttribution: daffie commentedMy idea was that we do it as a followup in this issue. It seams that that is not allowed, so here is the followup: #2389381: Impossible to add images in WYSIWYG including in-place editing due to fatal error
Comment #60
daffie CreditAttribution: daffie commentedThe issue has been fixed and the followup issue has also been fixed.
Comment #61
YesCT CreditAttribution: YesCT commentedI'm not sure why this was opened instead of using #2030641: Expand FilterFormat with methods but I closed that as a duplicate. It is a bit sad that the people who worked on that were not included in this.
Comment #62
YesCT CreditAttribution: YesCT commented