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

#2030587: Expand BreakpointGroup with methods

CommentFileSizeAuthor
#73 expand-breakpoint-with-methods-2030585-73.patch22.93 KBfilijonka
#73 interdiff-2030585-70-73.txt560 bytesfilijonka
#70 expand-breakpoint-with-methods-2030585-70.patch23.06 KBfilijonka
#70 interdiff-2030585-66-70.txt1.58 KBfilijonka
#66 expand-breakpoint-with-methods-2030585-66.patch22.22 KBfilijonka
#66 interdiff-2030585-63-66.txt2.54 KBfilijonka
#63 expand-breakpoint-with-methods-2030585-61.patch22.15 KBfilijonka
#61 expand-breakpoint-with-methods-2030585-61.patch3.07 KBmartin107
#59 expand-breakpoint-with-methods-2030585-59.patch23.4 KBczigor
#56 expand-breakpoint-with-methods-2030585-56.patch23.41 KBczigor
#51 expand-breakpoint-with-methods-2030585-51.patch23.4 KBpcambra
#50 expand-breakpoint-with-methods-2030585-50.patch23.52 KBSutharsan
#47 expand-breakpoint-with-methods-2030585-47.patch23.49 KBdaffie
#43 drupal8.breakpoint-module.2030585-43.patch21.66 KBJelle_S
#41 drupal8.breakpoint-module.2030585-41.patch21.89 KBJelle_S
#40 drupal8.breakpoint-module.2030585-40.patch23.09 KBJelle_S
#38 drupal8.breakpoint-module.2030585-38.patch23.17 KBmike.davis
#36 drupal8.breakpoint-module.2030585-36.patch22.39 KBmike.davis
#34 drupal8.breakpoint-module.2030585-34.patch23.75 KBmike.davis
#31 drupal8.breakpoint-module.2030585-31.patch21.1 KBmike.davis
#25 drupal8.breakpoint-module.2030585-25.patch38.33 KBmike.davis
#21 drupal8.breakpoint-module.2030585-21.patch34.64 KBmike.davis
#19 drupal8.breakpoint-module.2030585-19.patch11.52 KBcosmicdreams
#19 interdiff_2030585.txt11.08 KBcosmicdreams
#15 drupal8.breakpoint-module.2030585-15.patch4.32 KBkristofferwiklund
#13 drupal8.breakpoint-module.2030585-13.patch4.32 KBkristofferwiklund
#7 drupal8.breakpoint-module.2030585-7.patch4.28 KBkristofferwiklund
#7 interdiff-2030585-3-7.txt2.96 KBkristofferwiklund
#3 drupal8.breakpoint-module.2030585-3.patch3.17 KBJeroenT
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chrisjlee’s picture

Assigned: Unassigned » chrisjlee
joelpittet’s picture

Assigned: chrisjlee » Unassigned
Issue tags: +prague2013

Unassigned for drupalcon. @chrisjlee you're welcome to assign it back if you're going to grab it.

JeroenT’s picture

Added getters and setters for breakpoint entity

JeroenT’s picture

Status: Active » Needs review

Changing status to needs review...

kristofferwiklund’s picture

Assigned: Unassigned » kristofferwiklund

I will have a look.

joelpittet’s picture

Assigned: kristofferwiklund » Unassigned
Status: Needs review » Needs work
Issue tags: -prague2013

@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?

kristofferwiklund’s picture

Status: Needs work » Needs review
FileSize
2.96 KB
4.28 KB

I have reviewed it. And according to the meta issue:

do not duplicate already implemented functionality
Also (see #12),
do not add getter methods for things that already exist on EntityInterface, do not duplicate:

id() for the primary id of the entity. id() is already doing the same thing, so do not make getId().
getRevisionId() for the revision id (only relevant for entities that have revisions: node/custom block)
bundle() for the bundle (e.g. the type for nodes, vocabulary for terms, ...)
uuid()
language()

So I removed the duplicated methods.

I also added tests for the getters and setters methods.

Status: Needs review » Needs work
Issue tags: -Novice, -Entity Field API

The last submitted patch, drupal8.breakpoint-module.2030585-7.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Novice, +Entity Field API

The last submitted patch, drupal8.breakpoint-module.2030585-7.patch, failed testing.

mcrittenden’s picture

Seems 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

/core/modules/breakpoint/lib/Drupal/breakpoint/Tests/BreakpointAPITest.php

(note "/tests/" vs "/lib/")

So it's getting a "Cannot redeclare class" error.

kristofferwiklund’s picture

True. I had missed that PHPUnit och SimpleTest share the same namespace. I will rename my class.

kristofferwiklund’s picture

Status: Needs work » Needs review
FileSize
4.32 KB

Here comes a rename test class. And also renamed some variable names.

mcrittenden’s picture

Status: Needs review » Needs work

Still waiting on bot, but in the meantime, here are a few super nitpicky things.

  1. +++ b/core/modules/breakpoint/tests/Drupal/breakpoint/Tests/BreakpointMethodsTest.php
    @@ -0,0 +1,51 @@
    + * Tests for methods that sets and gets properties in a breakpoint.
    

    Should be "set and get"

  2. +++ b/core/modules/breakpoint/tests/Drupal/breakpoint/Tests/BreakpointMethodsTest.php
    @@ -0,0 +1,51 @@
    +    $breakpoint = new Breakpoint( array(), 'Breakpoint');
    

    Should not have a space before "array()". See https://drupal.org/coding-standards#constructors

  3. +++ b/core/modules/breakpoint/tests/Drupal/breakpoint/Tests/BreakpointMethodsTest.php
    @@ -0,0 +1,51 @@
    +    // i.e setLabel($randomdata) and getLabel()
    

    Should be "I.e.," (uppercase I and period and comma after the e).

  4. +++ b/core/modules/breakpoint/tests/Drupal/breakpoint/Tests/BreakpointMethodsTest.php
    @@ -0,0 +1,51 @@
    +      $this->assertEquals($value, $breakpoint->{'get'.$name}(), 'breakpoint_property: set'. $name .' and get'. $name);
    

    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.

kristofferwiklund’s picture

Status: Needs work » Needs review
FileSize
4.32 KB

For 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.

johnennew’s picture

Status: Needs review » Needs work
diff --git a/core/modules/breakpoint/lib/Drupal/breakpoint/Entity/Breakpoint.php b/core/modules/breakpoint/lib/Drupal/breakpoint/Entity/Breakpoint.php
+    /**
+     * @param \Drupal\breakpoint\Entity\weight $weight
+     */
+    public function setWeight($weight)
+    {
+        $this->weight = $weight;
+    }

I 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

+        $this->weight = $weight;

I think we use the $this->set('weight', $weight); for the set functions. Set functions should return the $this object to allow chaining.

+        return $this->weight;

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

mike.davis’s picture

Assigned: Unassigned » mike.davis

I'm going to start having a look at this now

cosmicdreams’s picture

@mike.davis: it's been a few days, can I take a shot at it?

cosmicdreams’s picture

Assigned: mike.davis » cosmicdreams
Status: Needs work » Needs review
FileSize
11.08 KB
11.52 KB

Here's my reformatting and hydration of the interface.

Status: Needs review » Needs work

The last submitted patch, drupal8.breakpoint-module.2030585-19.patch, failed testing.

mike.davis’s picture

Sorry for the delay, here is a patch for the interfaces

cosmicdreams’s picture

Dreditor 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?

mike.davis’s picture

Thanks, 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.

mike.davis’s picture

Thanks, 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.

mike.davis’s picture

OK, 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.

cosmicdreams’s picture

Status: Needs work » Needs review

Go testbot

Status: Needs review » Needs work

The last submitted patch, drupal8.breakpoint-module.2030585-25.patch, failed testing.

cosmicdreams’s picture

It appears that we may need to expand the capabilities of BreakpointGroup.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Entity/Breakpoint.phpundefined
@@ -284,4 +304,131 @@ public static function isValidMediaQuery($media_query) {
+  public function getName() {
+    return $this->get('name');
...
+  public function setLabel($label) {
+    $this->set('label', $label);
+    return $this;

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:

public function getName() {
   return $this->name;
}
public function setName($name) {
  return $this->name = $name
}
+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Entity/Breakpoint.phpundefined
@@ -284,4 +304,131 @@ public static function isValidMediaQuery($media_query) {
\ No newline at end of file

add extra line

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Entity/BreakpointGroup.phpundefined
@@ -161,16 +161,19 @@ public function addBreakpointFromMediaQuery($name, $media_query) {
-    $this->breakpoints[$breakpoint->id()] = $breakpoint;
+    $all_breakpoints = $this->getBreakpoints();
+    $all_breakpoints[$breakpoint->id()] = $breakpoint;

I see this work in multiple places. Maybe we need a setBreakpointById($id, $breakpoint) method

cosmicdreams’s picture

I 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.

mike.davis’s picture

Oh 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.

mike.davis’s picture

Here is an updated patch, with the BreakpointGroup changes removed and a fix for the test that failed.

JeroenT’s picture

Status: Needs work » Needs review

Marking as needs review.

Status: Needs review » Needs work

The last submitted patch, drupal8.breakpoint-module.2030585-31.patch, failed testing.

mike.davis’s picture

Assigned: cosmicdreams » mike.davis
Status: Needs work » Needs review
FileSize
23.75 KB

Update patch to fix issues from last test failure

The last submitted patch, drupal8.breakpoint-module.2030585-34.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

mike.davis’s picture

Updated patch to fix previous test fails.

mike.davis’s picture

Status: Needs work » Needs review

go testbot ....

mike.davis’s picture

Updated the comments on the interface to include the return value from the set methods.

balagan’s picture

Status: Needs review » Needs work

Needs to be rerolled, does not apply now.

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
23.09 KB

Rerolled patch

Jelle_S’s picture

From #2016679: Expand Entity Type interfaces to provide methods, protect the properties:

Ideally the visibility of the properties which are getters and setters in this case can force people to use the property directly and instead of using it directly, use it as a method so while working on the patch you can make the visibility as protected and change them back to public before RTBC - as stated in this comment

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))

mike.davis’s picture

Status: Needs review » Needs work

Thanks @Jelle_S, I had missed this comment item :), this patch looks good.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/BreakpointInterface.php
@@ -40,4 +40,109 @@ public function isValid();
+   * @return \Drupal\breakpoint\BreakpointInterface

Something that has been mentioned on other issues within this list, is that the method comments should return @self rather than the interface.

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
21.66 KB

New patch with comments according to #42. Thanks for the review ;-)

star-szr’s picture

Assigned: mike.davis » Unassigned
Status: Needs review » Needs work
Issue tags: +Needs reroll, +DrupalWorkParty

Unassigning and tagging for reroll.

@mike.davis or @Jelle_S if you want to work on this just reassign to yourself.

daffie’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 43: drupal8.breakpoint-module.2030585-43.patch, failed testing.

daffie’s picture

Status: Needs work » Needs review
FileSize
23.49 KB
daffie’s picture

Issue tags: -Needs reroll
RainbowArray’s picture

Patch no longer applies. Needs reroll.

Sutharsan’s picture

Patch #47 rerolled.

Patched failed because storage controller got removed from the breakpoint Entity definition.

 *   controllers = {
 *     "storage" = "Drupal\Core\Config\Entity\ConfigStorageController"
 *   },
pcambra’s picture

Status: Needs review » Needs work

The last submitted patch, 51: expand-breakpoint-with-methods-2030585-51.patch, failed testing.

Eli-T’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 51: expand-breakpoint-with-methods-2030585-51.patch, failed testing.

czigor’s picture

Assigned: Unassigned » czigor
czigor’s picture

Assigned: czigor » Unassigned
Status: Needs work » Needs review
FileSize
23.41 KB

Just a reroll of #51.

Status: Needs review » Needs work

The last submitted patch, 56: expand-breakpoint-with-methods-2030585-56.patch, failed testing.

czigor’s picture

Assigned: Unassigned » czigor
czigor’s picture

Assigned: czigor » Unassigned
Status: Needs work » Needs review
FileSize
23.4 KB

Status: Needs review » Needs work

The last submitted patch, 59: expand-breakpoint-with-methods-2030585-59.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
3.07 KB

Patch No longer applied -- Reroll.

filijonka’s picture

Status: Needs review » Needs work

This 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.

filijonka’s picture

Status: Needs work » Needs review
FileSize
22.15 KB

getLabel 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.

Status: Needs review » Needs work

The last submitted patch, 63: expand-breakpoint-with-methods-2030585-61.patch, failed testing.

filijonka’s picture

Assigned: Unassigned » filijonka
filijonka’s picture

Status: Needs work » Needs review
FileSize
2.54 KB
22.22 KB

ok so hopefully this will make the test

Status: Needs review » Needs work

The last submitted patch, 66: expand-breakpoint-with-methods-2030585-66.patch, failed testing.

filijonka’s picture

Status: Needs work » Needs review
filijonka’s picture

Assigned: filijonka » Unassigned

Ok 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.

filijonka’s picture

ok 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.

Status: Needs review » Needs work

The last submitted patch, 70: expand-breakpoint-with-methods-2030585-70.patch, failed testing.

filijonka’s picture

FYI 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.

filijonka’s picture

Status: Needs work » Needs review
FileSize
560 bytes
22.93 KB
dawehner’s picture

One 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.

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/BreakpointInterface.php
@@ -40,4 +40,137 @@
+   * @return self
+   *  The modified breakpoint
...
+   * @return self
+   *  The modified breakpoint
...
+   * @return self
+   *  The modified breakpoint
...
+   *
+   * @return self
+   *  The modified breakpoint
...
+   * @return self
+   *  The modified breakpoint
...
+   *
+   * @return self
+   *  The modified breakpoint
...
+   *
+   * @return self
+   *  The modified breakpoint

We do use now just @return $this without any additional explanation.

filijonka’s picture

well 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?

minneapolisdan’s picture

Issue tags: -Novice
eojthebrave’s picture

Status: Needs review » Needs work

This is going to need a re-roll for PSR-4 at a minimum.

filijonka’s picture

Assigned: Unassigned » filijonka
Issue tags: -DrupalWorkParty +Needs PSR-4
RainbowArray’s picture

Before 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.

attiks’s picture

Status: Needs work » Postponed

Postponing 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

filijonka’s picture

Assigned: filijonka » Unassigned
attiks’s picture

Status: Postponed » Closed (won't fix)