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())
postponed until this is solved: #2271529: Move breakpoint settings to theme and module *.breakpoints.yml files at root level if it is done this issue is irrelevant
Comment | File | Size | Author |
---|---|---|---|
#57 | interdiff-2030587-55-57.txt | 498 bytes | amitgoyal |
#57 | expand-breakpointgroup-with-methods-2030587-57.patch | 14.78 KB | amitgoyal |
Comments
Comment #1
plopescUps, change title
Comment #2
Michael Hodge Jr CreditAttribution: Michael Hodge Jr commentedComment #3
Michael Hodge Jr CreditAttribution: Michael Hodge Jr commentedComment #4
slv_ CreditAttribution: slv_ commentedI'll try and take a look to this.
Comment #5
xeniak CreditAttribution: xeniak commentedThis may (or may not :)) help.
Comment #6
xeniak CreditAttribution: xeniak commentedSummoning test-bot.
Comment #8
ReeceMarsland CreditAttribution: ReeceMarsland commentedI'm working on this as part of the prague sprint.
Comment #9
slv_ CreditAttribution: slv_ commentedEr... I was just on it (assigned yesterday)... re prague (see #4), but np. Will just walk away.
Comment #10
slv_ CreditAttribution: slv_ commentedComment #11
ReeceMarsland CreditAttribution: ReeceMarsland commentedThe error thrown during install:
Comment #12
xeniak CreditAttribution: xeniak commentedThanks, Reece. That helped.
Comment #14
slv_ CreditAttribution: slv_ commentedComment #15
rhm5000 CreditAttribution: rhm5000 commentedCode Cleanup:
Comment #17
rhm5000 CreditAttribution: rhm5000 commentedLine 53 of
/core/modules/breakpoint/lib/Drupal/breakpoint/Entity/BreakpointGroup.php
uses empty() with a method instead of a variable, empty() only supports variables, and will causes a phrase error prior to 5.5 see PHP: empty.Comment #19
xeniak CreditAttribution: xeniak commentedI've added getters and setters, but making the instance variables protected seems to keep the breakpoint group data from being saved. Any help/suggestions would be appreciated.
Comment #20
xeniak CreditAttribution: xeniak commentedRemoved setId() function.
Comment #21
johnennew CreditAttribution: johnennew commentedHi @xeniak - some coding standards items in the last patch need cleaning up.
Lines under an @return or @param need to be indented with two spaces - https://drupal.org/node/1354#return
This indentation is required.
The setters should return the $this object.
There should be a space after commas in a function call or elements of an array - https://drupal.org/coding-standards#functcall
No space after the opening parentheses of a control structure - https://drupal.org/coding-standards#controlstruct
Unary operators (operators that operate on only one value), such as !, should not have a space between the operator and the variable or number they are operating on - https://drupal.org/coding-standards#operators
You are reassigning the $breakpoints variable inside the for loop it is being iterated over in. I think its bad practice to change a variable being looped over within the loop itself like this.
Comment #22
xeniak CreditAttribution: xeniak commentedCeng, thanks. Corrections made. On that last one definitely comes under the heading "I don't know what I was thinking."
Comment #23
cosmicdreams CreditAttribution: cosmicdreams commentedI've asked this elsewhere but I'll ask this here too.
Why aren't we directly getting or setting properties from within the same object. Do we override the default get or set methods? Why can't we do this:
IMO, there shouldn't be a space between $breakpoint and "["
extra space
Comment #24
cosmicdreams CreditAttribution: cosmicdreams commentedI have found this logic in a few places. Maybe we need a setBreakpointById. (See picture module's tests)
Lastly
I don't think all of this work is worth it if you don't change the access modifiers of BreakpointGroup's properties to protected.
Comment #25
mike.davis CreditAttribution: mike.davis commentedI agree with @cosmicdreams on his last point, that the member variables should be defined as protected now that we are adding all the getters and setters.
With regards to the getters and setters, the idea with these is that you can daisy chain the setters together when calling them, so they should always return $this. The way to access the member variables is by using the available get() and set() methods as detailed in the CodeEntityBase class.
Comment #26
cosmicdreams CreditAttribution: cosmicdreams commentedThere is no CodeEntityBase in Drupal
Comment #27
mike.davis CreditAttribution: mike.davis commentedSorry, I meant ConfigEntityBase
Comment #28
balagan CreditAttribution: balagan commentedDoes not apply anymore, needs to be rerolled.
Comment #29
Jelle_SRerolled patch. Should pass all tests (it did on my local machine...)
Comment #31
Jelle_SFrom #2016679: Expand Entity Type interfaces to provide methods, protect the properties:
So changing the properties to their default before the patch (as otherwise we'd be introducing an API change (see comment #9 in that issue)
This should also answer #24 & #25
Comment #32
Jelle_SComment #33
Jelle_SForgot a small change in picture, this one should pass.
Comment #36
Jelle_SGrr, missed an other one.
Comment #37
uisc CreditAttribution: uisc commentedfine nice post. thanks
Comment #38
mike.davis CreditAttribution: mike.davis commentedThanks @Jelle_S, I had missed this comment item #9 :), this patch looks good.
Something that has been mentioned on other issues within this list, is that the method comments should @return self rather than the interface.
Comment #39
Jelle_SNew patch with comments according to #38. Thanks for the review ;-)
Comment #41
Jelle_SPlease behave testbot...
39: drupal8.breakpoint-module.2030587-39.patch queued for re-testing.
Comment #42
stuti.manandhar CreditAttribution: stuti.manandhar commented39: drupal8.breakpoint-module.2030587-39.patch queued for re-testing.
Comment #43
stuti.manandhar CreditAttribution: stuti.manandhar commentedComment #44
star-szrUnassigning and tagging for reroll.
@stuti.manandhar or @Jelle_S if you want to work on this just reassign to yourself.
Comment #45
daffie CreditAttribution: daffie commentedComment #46
RainbowArrayThis patch looks like it still applies if somebody wants to take a look and review this.
Comment #47
RainbowArrayHiding old patches.
Comment #48
pcambraRe roll after #2124377: Rename "Picture" module to "Responsive Image" module
Comment #49
filijonka CreditAttribution: filijonka commented48: expand-breakpointgroup-with-methods-2030587-48.patch queued for re-testing.
Comment #51
filijonka CreditAttribution: filijonka commentedrerolled #50 and changed some doc. Not sure I did the interdiffcorrectly but hopefully
Comment #52
dawehner@return $this
Why is there no getLabel() method here?
Comment #53
LinL CreditAttribution: LinL commented51: expand-breakpointgroup-with-methods-2030587-51.patch queued for re-testing.
Comment #55
amitgoyal CreditAttribution: amitgoyal commentedPatch in #51 no longer applies. Please review updated patch with changes in #52 - 1. Not sure about getLabel() method.
Comment #57
amitgoyal CreditAttribution: amitgoyal commentedPlease review updated patch for fixes in #55.
Comment #58
RainbowArrayIn all likelihood this issue is going to not be relevant if we get #2271529: Move breakpoint settings to theme and module *.breakpoints.yml files at root level done.
Comment #59
filijonka CreditAttribution: filijonka commentedpostponing this until #2271529: Move breakpoint settings to theme and module *.breakpoints.yml files at root level. If that is done we shall close this issue.
sorry @mdrummond and everyone else I forgott I was supposed to do this long time ago
Comment #60
attiks CreditAttribution: attiks commentedClosing since this is no longer relevant, see #2271529: Move breakpoint settings to theme and module *.breakpoints.yml files at root level