Part of meta-issue #2016679: [Meta] Expand Entity Type interfaces to provide methods.

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

Files: 
CommentFileSizeAuthor
#61 expand-breakpoint-with-methods-2030585-61.patch3.07 KBmartin107
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,673 pass(es).
[ View ]
#59 expand-breakpoint-with-methods-2030585-59.patch23.4 KBczigor
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,868 pass(es), 20 fail(s), and 0 exception(s).
[ View ]
#56 expand-breakpoint-with-methods-2030585-56.patch23.41 KBczigor
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,818 pass(es), 27 fail(s), and 0 exception(s).
[ View ]
#51 expand-breakpoint-with-methods-2030585-51.patch23.4 KBpcambra
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch expand-breakpoint-with-methods-2030585-51.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#50 expand-breakpoint-with-methods-2030585-50.patch23.52 KBSutharsan
PASSED: [[SimpleTest]]: [MySQL] 64,730 pass(es).
[ View ]
#47 expand-breakpoint-with-methods-2030585-47.patch23.49 KBdaffie
PASSED: [[SimpleTest]]: [MySQL] 59,827 pass(es).
[ View ]
#36 drupal8.breakpoint-module.2030585-36.patch22.39 KBmike.davis
PASSED: [[SimpleTest]]: [MySQL] 59,950 pass(es).
[ View ]
#34 drupal8.breakpoint-module.2030585-34.patch23.75 KBmike.davis
FAILED: [[SimpleTest]]: [MySQL] 59,720 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#31 drupal8.breakpoint-module.2030585-31.patch21.1 KBmike.davis
FAILED: [[SimpleTest]]: [MySQL] 59,567 pass(es), 20 fail(s), and 0 exception(s).
[ View ]
#25 drupal8.breakpoint-module.2030585-25.patch38.33 KBmike.davis
FAILED: [[SimpleTest]]: [MySQL] 59,054 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#21 drupal8.breakpoint-module.2030585-21.patch34.64 KBmike.davis
FAILED: [[SimpleTest]]: [MySQL] 59,051 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#19 drupal8.breakpoint-module.2030585-19.patch11.52 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#19 interdiff_2030585.txt11.08 KBcosmicdreams
#15 drupal8.breakpoint-module.2030585-15.patch4.32 KBkristofferwiklund
PASSED: [[SimpleTest]]: [MySQL] 58,727 pass(es).
[ View ]
#13 drupal8.breakpoint-module.2030585-13.patch4.32 KBkristofferwiklund
FAILED: [[SimpleTest]]: [MySQL] 58,665 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#7 drupal8.breakpoint-module.2030585-7.patch4.28 KBkristofferwiklund
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#7 interdiff-2030585-3-7.txt2.96 KBkristofferwiklund
#3 drupal8.breakpoint-module.2030585-3.patch3.17 KBJeroenT
PASSED: [[SimpleTest]]: [MySQL] 58,601 pass(es).
[ View ]

Comments

Assigned:Unassigned» chrisjlee

Assigned:chrisjlee» Unassigned
Issue tags:+prague2013

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

StatusFileSize
new3.17 KB
PASSED: [[SimpleTest]]: [MySQL] 58,601 pass(es).
[ View ]

Added getters and setters for breakpoint entity

Status:Active» Needs review

Changing status to needs review...

Assigned:Unassigned» kristofferwiklund

I will have a look.

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?

Status:Needs work» Needs review
StatusFileSize
new2.96 KB
new4.28 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

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.

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.

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.

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

Status:Needs work» Needs review
StatusFileSize
new4.32 KB
FAILED: [[SimpleTest]]: [MySQL] 58,665 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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

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.

Status:Needs work» Needs review
StatusFileSize
new4.32 KB
PASSED: [[SimpleTest]]: [MySQL] 58,727 pass(es).
[ View ]

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.

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

Assigned:Unassigned» mike.davis

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

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

Assigned:mike.davis» cosmicdreams
Status:Needs work» Needs review
StatusFileSize
new11.08 KB
new11.52 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

StatusFileSize
new34.64 KB
FAILED: [[SimpleTest]]: [MySQL] 59,051 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

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

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?

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.

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.

StatusFileSize
new38.33 KB
FAILED: [[SimpleTest]]: [MySQL] 59,054 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review

Go testbot

Status:Needs review» Needs work

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

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

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.

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.

StatusFileSize
new21.1 KB
FAILED: [[SimpleTest]]: [MySQL] 59,567 pass(es), 20 fail(s), and 0 exception(s).
[ View ]

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

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.

Assigned:cosmicdreams» mike.davis
Status:Needs work» Needs review
StatusFileSize
new23.75 KB
FAILED: [[SimpleTest]]: [MySQL] 59,720 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

Update patch to fix issues from last test failure

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

Issue summary:View changes

Updated issue summary.

Issue summary:View changes
StatusFileSize
new22.39 KB
PASSED: [[SimpleTest]]: [MySQL] 59,950 pass(es).
[ View ]

Updated patch to fix previous test fails.

Status:Needs work» Needs review

go testbot ....

StatusFileSize
new23.17 KB
PASSED: [[SimpleTest]]: [MySQL] 59,676 pass(es).
[ View ]

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

Status:Needs review» Needs work

Needs to be rerolled, does not apply now.

Status:Needs work» Needs review
StatusFileSize
new23.09 KB
PASSED: [[SimpleTest]]: [MySQL] 59,009 pass(es).
[ View ]

Rerolled patch

StatusFileSize
new21.89 KB
PASSED: [[SimpleTest]]: [MySQL] 59,076 pass(es).
[ View ]

From #2016679: [Meta] Expand Entity Type interfaces to provide methods:

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

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.

Status:Needs work» Needs review
StatusFileSize
new21.66 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.breakpoint-module.2030585-43.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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.

Status:Needs work» Needs review

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new23.49 KB
PASSED: [[SimpleTest]]: [MySQL] 59,827 pass(es).
[ View ]

Issue tags:-Needs reroll

Issue tags:+Needs reroll

Patch no longer applies. Needs reroll.

Issue tags:-Needs reroll
StatusFileSize
new23.52 KB
PASSED: [[SimpleTest]]: [MySQL] 64,730 pass(es).
[ View ]

Patch #47 rerolled.

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

*   controllers = {
*     "storage" = "Drupal\Core\Config\Entity\ConfigStorageController"
*   },

StatusFileSize
new23.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch expand-breakpoint-with-methods-2030585-51.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re roll after renaming Picture module in #2124377: Rename "Picture" module to "Responsive Image" module

Status:Needs review» Needs work

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

Status:Needs work» Needs review

Status:Needs review» Needs work

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

Assigned:Unassigned» czigor

Assigned:czigor» Unassigned
Status:Needs work» Needs review
StatusFileSize
new23.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,818 pass(es), 27 fail(s), and 0 exception(s).
[ View ]

Just a reroll of #51.

Status:Needs review» Needs work

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

Assigned:Unassigned» czigor

Assigned:czigor» Unassigned
Status:Needs work» Needs review
StatusFileSize
new23.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,868 pass(es), 20 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new3.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,673 pass(es).
[ View ]

Patch No longer applied -- Reroll.