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 |
---|---|---|---|
#58 | interdiff-2030661-58.txt | 473 bytes | lokapujya |
#58 | 2030661-58.patch | 5.74 KB | lokapujya |
#56 | interdiff-2030661-56.txt | 441 bytes | lokapujya |
#56 | 2030661-56.patch | 5.75 KB | lokapujya |
#54 | interdiff-2030661-54.txt | 496 bytes | lokapujya |
Comments
Comment #1
chrisjlee CreditAttribution: chrisjlee commentedComment #2
YesCT CreditAttribution: YesCT commented@chrisjlee did you have any questions or intermediate work to post?
Comment #3
chrisjlee CreditAttribution: chrisjlee commentedYes. So would i usually (or most of the time) be adding two methods ->settings() and ->getSettings()
Comment #4
Ivan Zugec CreditAttribution: Ivan Zugec commentedHere is my first pass.
Comment #5
daffie CreditAttribution: daffie commented4: tour_methods-2030661-4.patch queued for re-testing.
I did a review and everything looks ok.
I requested a re-test to make sure that everything is still good. If so, then I shall give this issue a rtbc.
Comment #7
daffie CreditAttribution: daffie commented4: tour_methods-2030661-4.patch queued for re-testing.
Comment #8
daffie CreditAttribution: daffie commentedComment #10
daffie CreditAttribution: daffie commentedComment #11
dawehnerGiven that tips are objects we can use a syntax like \Drupal\tour\TipPluginInterface[] instead of "array", so tools can leverage this information.
I guess we should use the TourInterface instead here.
Afaik this lines should be proper sentences as well.
s/Return/Returns the
Comment #12
larowlanre-roll plus creates new tipbag in setTips method, else I don't think it would work.
Comment #13
dawehnerNote: it is @return $this now.
Comment #15
larowlanFixes docblock.
Comment #16
BerdirThat's weird? :)
Comment #17
larowlanFixes #16
Comment #18
larowlanand interdiff
Comment #19
BerdirLooks good.
Comment #20
alexpottCan this be protected now?
Why are we not adding any usages of these functions?
Comment #21
alexpottChanging status accordingly.
Comment #22
izus CreditAttribution: izus commentedHi,
here is the patch following #20 recommandations.
Thanks
Comment #24
lokapujyaReroll
Comment #26
lokapujyaValue array ( 0 => 'tour_test', ) is equal to value array ( 0 => 'system', 1 => 'tour_test', ).
The test probably shouldn't expect 'system' anymore? I don't know why that changed though.
Comment #27
l0keRemoving 'system' from array fixes failing test.
Comment #28
dawehnerLet's use {@inheritdoc}
Comment #29
l0keThank you, changed to {@inheritdoc}.
Comment #30
dawehnerThanks
Comment #31
alexpottCan we protect all the tour properties? Thanks.
These have no external usages are therefore unnecessary.
This is only used in a test so kind of fake
This is totally unused code now.
Comment #32
l0keChanged all properties to protected, removed unnecessary methods.
Comment #33
m1r1k CreditAttribution: m1r1k commentedComment #34
dinarcon CreditAttribution: dinarcon commented@lokeoke's patch applies cleanly.
In the code there are calls to the drupal_clean_css_identifier() function which has been marked as deprecated according to the docs. Shall we update the function call here or a mass update of the function call will be done later?
Comment #35
larowlanyeah let's fix those whilst we're touching those bits.
Comment #36
dinarcon CreditAttribution: dinarcon commentedOk, @larowlan. Replacing drupal_clean_css_identifier() calls in @lokeoke's patch.
Comment #37
larowlantoo many tos? The module to which this tour belongs - or - the module this tour belongs to?
Other than that, looks RTBC
Comment #38
dinarcon CreditAttribution: dinarcon commentedNew patch with @larowlan suggestion in #37.
Comment #39
larowlanThanks @dinarcon
Comment #40
Wim LeersThis could be just
->getPluginId()
.I think this should be cleaned up still;
TipPluginInterface
should have agetId()
(orid()
) method, just like it already hasgetLabel()
. But I guess that's out of scope… is there an issue for that yet, @larowlan?Back to NR (not NW) so larowlan can determine whether this feedback should be addressed here or not.
Comment #41
larowlanYep you're right, let's fix it here - thanks!
Comment #42
l0kePatch with @Wim Leers notes in #40.
Comment #44
lokapujyaThe method doesn't exist yet; It needs to be created.
Comment #45
l0keAdded methods and some cleanup in docs.
Comment #46
lokapujyaComment #47
Wim LeersLooks good! :)
Comment #48
alexpottDo we actually need this? Is not the method on PluginBase good enough?
Also can we get a followup to remove the confusing unused properties from TipPluginBase, TipPluginText, and TipPluginImage which all have these properties that afaics are never used since ->get() gets them from the configuration property.
Comment #50
daffie CreditAttribution: daffie commentedThe patch probably needs a reroll.
Why is this function necessary.
Comment #51
lokapujyaI'm not sure. Because it seems like the id of the tip, not the id of the plugin.
Comment #52
daffie CreditAttribution: daffie commentedThe problem is that the class TipPluginBase does not have a variable "id". As far as I can see.
@lokapujya: If you update the patch. I shall do the review.
Comment #53
lokapujyaRight, I assume that the id comes from the tours .yml file.
We probably should do the change in 48.
Comment #54
lokapujyatrying out #48.
Comment #55
lokapujyaI think I was supposed to remove it from TipPluginBase too. Will try that later.
Comment #56
lokapujyaTipPluginBase. *Ignore this patch*
Comment #58
lokapujyaIgnore the last patch, wrong change.
Comment #59
daffie CreditAttribution: daffie commented@lokapujya: I think that something is wrong with your patch file, because it is not being tested.
Comment #60
lokapujyakick off the test.
Comment #61
daffie CreditAttribution: daffie commentedAll the class variables for Tour and TipPluginBase are protected.
The comments and the doc block are in order.
For the Tour class is the function getModule() added.
For the TipPluginBase is the function id() added.
There are no tests added for the two functions.
The patch is given all green from the test-server.
In the TourViewBuilder class there is a bit of code cleaning with the replacement of drupal_clean_css_identifier.
The patch get a RTBC from me.
Comment #62
alexpottThe meta issue #2016679: Expand Entity Type interfaces to provide methods, protect the properties has the beta evaluation form completed. Committed 737f04d and pushed to 8.0.x. Thanks!