Expand BreakpointGroup with methods

I'll try and take a look to this.

This may (or may not :)) help.

Summoning test-bot.

I'm working on this as part of the prague sprint.

Er... I was just on it (assigned yesterday)... re prague (see #4), but np. Will just walk away.

The error thrown during install:

Recoverable fatal error: Argument 1 passed to Drupal\breakpoint\Entity\BreakpointGroup::setBreakpoints() must be an array, null given, called in /projects/drupal8/www_2030587/core/modules/breakpoint/lib/Drupal/breakpoint/Entity/BreakpointGroup.php on line 225 and defined in Drupal\breakpoint\Entity\BreakpointGroup->setBreakpoints() (line 100 of /projects/drupal8/www_2030587/core/modules/breakpoint/lib/Drupal/breakpoint/Entity/BreakpointGroup.php).

Thanks, Reece. That helped.

Code Cleanup:

• Removed extra spaces near conditionals and method parameters.
• Added comma to last element in array

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

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

Removed setId() function.

Hi @xeniak - some coding standards items in the last patch need cleaning up.

+  /**
    * Checks if the breakpoint group is valid.
    *
    * @throws \Drupal\breakpoint\InvalidBreakpointSourceTypeException
    * @throws \Drupal\breakpoint\InvalidBreakpointSourceException
    *
    * @return bool
-   *   Returns TRUE if the breakpoint group is valid.
+   * Returns TRUE if the breakpoint group is valid.
    */

Lines under an @return or @param need to be indented with two spaces - https://drupal.org/node/1354#return

diff --git a/core/modules/breakpoint/lib/Drupal/breakpoint/Entity/BreakpointGroup.php b/core/modules/breakpoint/lib/Drupal/breakpoint/Entity/BreakpointGroup.php
index 774890d..1d2838a 100644
--- a/core/modules/breakpoint/lib/Drupal/breakpoint/Entity/BreakpointGroup.php
+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Entity/BreakpointGroup.php
@@ -18,22 +18,21 @@
  * Defines the BreakpointGroup entity.
  *
  * @EntityType(
- *   id = "breakpoint_group",
- *   label = @Translation("Breakpoint group"),
- *   module = "breakpoint",
- *   controllers = {
- *     "storage" = "Drupal\Core\Config\Entity\ConfigStorageController"
- *   },
- *   config_prefix = "breakpoint.breakpoint_group",
- *   entity_keys = {
- *     "id" = "id",
- *     "label" = "label",
- *     "uuid" = "uuid"
- *   }
+ * id = "breakpoint_group",
+ * label = @Translation("Breakpoint group"),
+ * module = "breakpoint",
+ * controllers = {
+ * "storage" = "Drupal\Core\Config\Entity\ConfigStorageController"
+ * },
+ * config_prefix = "breakpoint.breakpoint_group",
+ * entity_keys = {
+ * "id" = "id",
+ * "label" = "label",
+ * "uuid" = "uuid"
+ * }
  * )
  */

This indentation is required.

+  /**
+   * {@inheritdoc}
+   */
+  public function setName($name) {
+    return $this->set('name',$name);
+  }

The setters should return the $this object.

+    if (preg_match('/[^a-z0-9_]+/',$name || empty($name))) {

There should be a space after commas in a function call or elements of an array - https://drupal.org/coding-standards#functcall

+    foreach ( $breakpoints as $breakpoint_name ) {

No space after the opening parentheses  of a control structure - https://drupal.org/coding-standards#controlstruct

+      if (! $breakpoint) {

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

   public function addBreakpoints($breakpoints) {
-    foreach ($breakpoints as $breakpoint_name) {
+    foreach ( $breakpoints as $breakpoint_name ) {
       // Check if breakpoint exists, assume $breakpoint_name is a machine name.
-      $breakpoint = entity_load('breakpoint', $this->sourceType . '.' . $this->source . '.' . $breakpoint_name);
+      $breakpoint = entity_load('breakpoint',$this->getSourceType() . '.' . $this->getSource() . '.' . $breakpoint_name);
       // If the breakpoint doesn't exist, assume $breakpoint_name is an id.
-      if (!$breakpoint) {
-        $breakpoint = entity_load('breakpoint', $breakpoint_name);
+      if (! $breakpoint) {
+        $breakpoint = entity_load('breakpoint',$breakpoint_name);
       }
       // If the breakpoint doesn't exists, do not add it.
       if ($breakpoint) {
         // Add breakpoint to group.
-        $this->breakpoints[$breakpoint->id()] = $breakpoint;
+        $breakpoints = $this->getBreakpoints();
+        $breakpoints [$breakpoint->id()] = $breakpoint;
+        $this->setBreakpoints($breakpoints);
       }
     }
   }

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.

Ceng, thanks. Corrections made. On that last one definitely comes under the heading "I don't know what I was thinking."

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Entity/BreakpointGroup.phpundefined
@@ -102,6 +102,74 @@ public function __construct(array $values, $entity_type) {
   /**
+   * {@inheritdoc}
+   */
+  public function getName() {
+    return $this->get('name');
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function setName($name) {
+    $this->set('name', $name);
+    return $this;

I'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:

/**
 * {@inheritdoc}
 */
public function getName() {
  return $this->name;
}

/**
 * {@inheritdoc}
 */
public function setName($name) {
  $this->name =  $name;
  return $this;
}

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Entity/BreakpointGroup.phpundefined
@@ -148,56 +222,64 @@ public function isValid() {
+    $breakpoints [$breakpoint->id()] = $breakpoint;

IMO, there shouldn't be a space between $breakpoint and "["

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Entity/BreakpointGroup.phpundefined
@@ -148,56 +222,64 @@ public function isValid() {
+    ¶
...
+        $new_breakpoints [$breakpoint->id()] = $breakpoint;        ¶

extra space

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Entity/BreakpointGroup.phpundefined
@@ -148,56 +222,64 @@ public function isValid() {
-    $this->breakpoints[$breakpoint->id()] = $breakpoint;
+    $breakpoints = $this->getBreakpoints();
+    $breakpoints [$breakpoint->id()] = $breakpoint;
+    $this->setBreakpoints($breakpoints);

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

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

There is no CodeEntityBase in Drupal

Sorry, I meant ConfigEntityBase

Does not apply anymore, needs to be rerolled.

Rerolled patch. Should pass all tests (it did on my local machine...)

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)

This should also answer #24 & #25

Forgot a small change in picture, this one should pass.

Grr, missed an other one.

fine nice post. thanks

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

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/BreakpointGroupInterface.php
@@ -15,6 +15,99 @@
+   * @return \Drupal\breakpoint\BreakpointGroup\Interface

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

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

Please behave testbot...
39: drupal8.breakpoint-module.2030587-39.patch queued for re-testing.

39: drupal8.breakpoint-module.2030587-39.patch queued for re-testing.

Unassigning and tagging for reroll.

@stuti.manandhar or @Jelle_S if you want to work on this just reassign to yourself.

This patch looks like it still applies if somebody wants to take a look and review this.

Hiding old patches.

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

48: expand-breakpointgroup-with-methods-2030587-48.patch queued for re-testing.

rerolled #50 and changed some doc. Not sure I did the interdiffcorrectly but hopefully

1. +++ b/core/modules/breakpoint/lib/Drupal/breakpoint/BreakpointGroupInterface.php
   @@ -25,6 +25,99 @@
   +   * @return self
   +   *   The called breakpoint group entity
   ...
   +   * @return self
   +   *   The called breakpoint group entity
   ...
   +   * @return self
   +   *   The called breakpoint group entity
   ...
   +   * @return self
   +   *   The called breakpoint group entity
   

   @return $this

2. +++ b/core/modules/breakpoint/lib/Drupal/breakpoint/BreakpointGroupInterface.php
   @@ -25,6 +25,99 @@
   +  public function setLabel($label);
   

   Why is there no getLabel() method here?

51: expand-breakpointgroup-with-methods-2030587-51.patch queued for re-testing.

Patch in #51 no longer applies. Please review updated patch with changes in #52 - 1. Not sure about getLabel() method.

Please review updated patch for fixes in #55.

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

postponing 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

Closing since this is no longer relevant, see #2271529: Move breakpoint settings to theme and module *.breakpoints.yml files at root level