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())
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#73 | expand-breakpoint-with-methods-2030585-73.patch | 22.93 KB | filijonka |
#73 | interdiff-2030585-70-73.txt | 560 bytes | filijonka |
#70 | expand-breakpoint-with-methods-2030585-70.patch | 23.06 KB | filijonka |
#70 | interdiff-2030585-66-70.txt | 1.58 KB | filijonka |
#66 | expand-breakpoint-with-methods-2030585-66.patch | 22.22 KB | filijonka |
Comments
Comment #1
chrisjlee CreditAttribution: chrisjlee commentedComment #2
joelpittetUnassigned for drupalcon. @chrisjlee you're welcome to assign it back if you're going to grab it.
Comment #3
JeroenTAdded getters and setters for breakpoint entity
Comment #4
JeroenTChanging status to needs review...
Comment #5
kristofferwiklund CreditAttribution: kristofferwiklund commentedI will have a look.
Comment #6
joelpittet@jeroen12345 Thanks for the patch. I'm not sure on this issue exactly what needs doing so as far as that I'll leave that to someone else. I'll just nitpick that you have indented with 4 spaces instead of 2 as per the drupal coding standards. So a new patch will be needed.
@kristofferwiklund did you get a chance to review the new getters/setters are as expected?
Comment #7
kristofferwiklund CreditAttribution: kristofferwiklund commentedI have reviewed it. And according to the meta issue:
So I removed the duplicated methods.
I also added tests for the getters and setters methods.
Comment #9
joelpittet#7: drupal8.breakpoint-module.2030585-7.patch queued for re-testing.
Comment #11
mcrittenden CreditAttribution: mcrittenden commentedSeems like it's failing because it's trying to add a new
BreakpointAPITest
here:/core/modules/breakpoint/tests/Drupal/breakpoint/Tests/BreakpointAPITest.php
...but that class already exists in HEAD at
(note "/tests/" vs "/lib/")
So it's getting a "Cannot redeclare class" error.
Comment #12
kristofferwiklund CreditAttribution: kristofferwiklund commentedTrue. I had missed that PHPUnit och SimpleTest share the same namespace. I will rename my class.
Comment #13
kristofferwiklund CreditAttribution: kristofferwiklund commentedHere comes a rename test class. And also renamed some variable names.
Comment #14
mcrittenden CreditAttribution: mcrittenden commentedStill waiting on bot, but in the meantime, here are a few super nitpicky things.
Should be "set and get"
Should not have a space before "array()". See https://drupal.org/coding-standards#constructors
Should be "I.e.," (uppercase I and period and comma after the e).
Could be wrong here, but I think something like "Test Breakpoint property:" should be used in place of "breakpoint_property:" in a human readable test description.
Comment #15
kristofferwiklund CreditAttribution: kristofferwiklund commentedFor the failing of SimpleTest I can not see what it is in my code.
For the the feedback in #14:
1 + 3: Thanks. English is not my first language so I need feedback on that.
2: Typo
4: Copy and cut from a SimpleTest (I started there and converted to PHPUnit). Have renamed it now.
Comment #16
johnennew CreditAttribution: johnennew commentedI think these functions need to be added to the BreakpointInterface then implemented in the concrete Breakpoint class. In the Breakpoint the get and set functions just get an {@inheritdoc} for the comment.
Check the Drupal Coding Standards (DCS), specifically:
* Function declarations (opening bracket should be on the same line as the function declaration): https://drupal.org/coding-standards#functdecl
* Function doc blocks should follow these rules - https://drupal.org/node/1354#functions
* Two space indent on new lines - https://drupal.org/coding-standards#indenting
I think we use the $this->set('weight', $weight); for the set functions. Set functions should return the $this object to allow chaining.
Similarly, I think the get functions should use return $this->get('weight');
Check one of the other patches in the meta issue for examples of style:
https://drupal.org/node/2016679
Comment #17
mike.davis CreditAttribution: mike.davis commentedI'm going to start having a look at this now
Comment #18
cosmicdreams CreditAttribution: cosmicdreams commented@mike.davis: it's been a few days, can I take a shot at it?
Comment #19
cosmicdreams CreditAttribution: cosmicdreams commentedHere's my reformatting and hydration of the interface.
Comment #21
mike.davis CreditAttribution: mike.davis commentedSorry for the delay, here is a patch for the interfaces
Comment #22
cosmicdreams CreditAttribution: cosmicdreams commentedDreditor doesn't seem to be working for me so I'll have to be overly-broad in this comment.
Take a look at the patch I created in #19. It has some good suggestions on how to fix the documentation issues in the patch from #21. I think if we can combine the patches from those two comments we're 90% done. I completely overlooked the BreakpointGroup and that documentation needs to be fixed as well.
Why are you getting rid of the $id and $name for both Breakpoint and BreakpointGroup?
Comment #23
mike.davis CreditAttribution: mike.davis commentedThanks, I have just been having a look through your patch from #19 and I agree we can use the documentation from your patch to cover off most of the work. I'll have a look at updating it tonight and re-post a new patch.
I removed the $id, $uuid as I thought that these were defined within the main Entity class where the main id(), uuid() methods are based, but it seems that although the methods are defined the member variables aren't.
Comment #24
mike.davis CreditAttribution: mike.davis commentedThanks, I have just been having a look through your patch from #19 and I agree we can use the documentation from your patch to cover off most of the work. I'll have a look at updating it tonight and re-post a new patch.
I removed the $id, $uuid as I thought that these were defined within the main Entity class where the main id(), uuid() methods are based, but it seems that although the methods are defined the member variables aren't.
Comment #25
mike.davis CreditAttribution: mike.davis commentedOK, here is a combined patch from #19 and #21. I have also added back the member variables for $uid and $uuid so that all the properties are at least defined.
Comment #26
cosmicdreams CreditAttribution: cosmicdreams commentedGo testbot
Comment #28
cosmicdreams CreditAttribution: cosmicdreams commentedIt appears that we may need to expand the capabilities of BreakpointGroup.
In general, when we're creating getters and setters for properties that are owned by the object, why couldn't we directly access and modify the property?
For example:
add extra line
I see this work in multiple places. Maybe we need a setBreakpointById($id, $breakpoint) method
Comment #29
cosmicdreams CreditAttribution: cosmicdreams commentedI just found #2030587: Expand BreakpointGroup with methods. So this patch seems to be doing too much. We shouldn't include changes to BreakpointGroup. Which would punt the issue of whether we need setBreakpointById to that issue. And make this issue go green.
Comment #30
mike.davis CreditAttribution: mike.davis commentedOh I didn't see this issue, seems like I've unnecessarily made more work for myself :). Looks like the patch for breakpoint group is mostly covered from what I can see of it.
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 ConfigEntityBase class.
I'll remove the additional changes for the breakpoint group and this should fix the error that the test failed on (as this was to do with a breakpoint group property reference), then hopefully we will be nearly there with this one.
Comment #31
mike.davis CreditAttribution: mike.davis commentedHere is an updated patch, with the BreakpointGroup changes removed and a fix for the test that failed.
Comment #32
JeroenTMarking as needs review.
Comment #34
mike.davis CreditAttribution: mike.davis commentedUpdate patch to fix issues from last test failure
Comment #35.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #36
mike.davis CreditAttribution: mike.davis commentedUpdated patch to fix previous test fails.
Comment #37
mike.davis CreditAttribution: mike.davis commentedgo testbot ....
Comment #38
mike.davis CreditAttribution: mike.davis commentedUpdated the comments on the interface to include the return value from the set methods.
Comment #39
balagan CreditAttribution: balagan commentedNeeds to be rerolled, does not apply now.
Comment #40
Jelle_SRerolled patch
Comment #41
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))
Comment #42
mike.davis CreditAttribution: mike.davis commentedThanks @Jelle_S, I had missed this comment item :), 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 #43
Jelle_SNew patch with comments according to #42. Thanks for the review ;-)
Comment #44
star-szrUnassigning and tagging for reroll.
@mike.davis or @Jelle_S if you want to work on this just reassign to yourself.
Comment #45
daffie CreditAttribution: daffie commented43: drupal8.breakpoint-module.2030585-43.patch queued for re-testing.
Comment #47
daffie CreditAttribution: daffie commentedComment #48
daffie CreditAttribution: daffie commentedComment #49
RainbowArrayPatch no longer applies. Needs reroll.
Comment #50
Sutharsan CreditAttribution: Sutharsan commentedPatch #47 rerolled.
Patched failed because storage controller got removed from the breakpoint Entity definition.
Comment #51
pcambraRe roll after renaming Picture module in #2124377: Rename "Picture" module to "Responsive Image" module
Comment #53
Eli-T51: expand-breakpoint-with-methods-2030585-51.patch queued for re-testing.
Comment #55
czigor CreditAttribution: czigor commentedComment #56
czigor CreditAttribution: czigor commentedJust a reroll of #51.
Comment #58
czigor CreditAttribution: czigor commentedComment #59
czigor CreditAttribution: czigor commentedComment #61
martin107 CreditAttribution: martin107 commentedPatch No longer applied -- Reroll.
Comment #62
filijonka CreditAttribution: filijonka commentedThis latest reroll can't be right. We had a working patch in #50 with get/set methods and nothing of that is left in the latest patch.
Comment #63
filijonka CreditAttribution: filijonka commentedgetLabel and setLabel is a bit confusing since we have a function label() that the interface tells us to implement.
based on #50. Not sure if the test will apply since it's apparently been some problems before this patch with that but since the previous patch isn't correct I felt I wanted one up that is more correct.
Comment #65
filijonka CreditAttribution: filijonka commentedComment #66
filijonka CreditAttribution: filijonka commentedok so hopefully this will make the test
Comment #68
filijonka CreditAttribution: filijonka commented66: expand-breakpoint-with-methods-2030585-66.patch queued for re-testing.
Comment #69
filijonka CreditAttribution: filijonka commentedOk just some notes from me about this latest patch that even though it passed the test I'm not to keen on what I had to do to make it pass.
Several protected properties had to be declared as public, I thought that the toArray() in the interface would make it possible to have protected properties.
Comment #70
filijonka CreditAttribution: filijonka commentedok I got it working locally with protected properties again by adding the function toArray(), thought that the parent::toarray should be naough but it wasn't.
Comment #72
filijonka CreditAttribution: filijonka commentedFYI the test that the bot is complaining about is failing due to that the property $multipliers is protected, trying to figure out why that's the reason why there is no new patch yet.
Comment #73
filijonka CreditAttribution: filijonka commentedComment #74
dawehnerOne thing I wonder about. Why do we provide both a getLabel as well as a label() method. Yes there is a semantic difference. I know we do that for node as well.
We do use now just @return $this without any additional explanation.
Comment #75
filijonka CreditAttribution: filijonka commentedwell we ask for get/set functions and people do that. just label() is not logical name for a get function so people probably missed that.
This issue is discussing about renaming #2246695: replace all core usages of id() with getid() id and label to get* instead so felt ok to leave this as it is.
The second part i'm not sure i'm following yoour concern; we modify with set functions and return the modified self.
And that is what the comment says doesn't it?
Comment #76
minneapolisdan CreditAttribution: minneapolisdan commentedComment #77
eojthebraveThis is going to need a re-roll for PSR-4 at a minimum.
Comment #78
filijonka CreditAttribution: filijonka commentedComment #79
RainbowArrayBefore re-rolling this, we should check if this is still relevant given the re-architecting going on in #2271529: Move breakpoint settings to theme and module *.breakpoints.yml files at root level.
Comment #80
attiks CreditAttribution: attiks commentedPostponing since the whole system of breakpoints is going to change, see #2271529: Move breakpoint settings to theme and module *.breakpoints.yml files at root level
Comment #81
filijonka CreditAttribution: filijonka commentedComment #82
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