As a follow up to #1826602: Allow all configuration entities to be enabled/disabled, Let's add a setStatus() method on configuration entities. enable/disable methods would then just wrap this.

Here is an initial patch. It just adds this as a method to ConfigEntityBase, no interfaces etc..

I guess the question is do we want this to be a part of the ConfigEntityInterface? This could mean all of them, or remove enable/disable and just have setStatus in the interface.

Files: 
CommentFileSizeAuthor
#16 1911814-16.patch1.88 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1911814-16.patch. Unable to apply patch. See the log in the details link for more information. View
#16 interdiff.txt549 bytesdamiankloip
#15 drupal-1911814-15.patch1.84 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1911814-15.patch. Unable to apply patch. See the log in the details link for more information. View
#15 interdiff.txt551 bytesdawehner
#14 drupal-1911814-14.patch1.87 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1911814-14.patch. Unable to apply patch. See the log in the details link for more information. View
#14 interdiff.txt390 bytesdawehner
#13 drupal-1911814-13.patch1.88 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1911814-13.patch. Unable to apply patch. See the log in the details link for more information. View
#12 drupal-1911814-12.patch4.59 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1911814-12.patch. Unable to apply patch. See the log in the details link for more information. View
#11 1911814-11.setStatus-method.patch1.16 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1911814-11.setStatus-method.patch. Unable to apply patch. See the log in the details link for more information. View
#7 1911814-7.patch1.7 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1911814-7.patch. Unable to apply patch. See the log in the details link for more information. View
#2 1911814-2.patch1.7 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 49,212 pass(es), 524 fail(s), and 254 exception(s). View
#2 interdiff.txt1.39 KBdamiankloip
d8.setStatus.patch1.05 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 49,984 pass(es), 1 fail(s), and 24 exception(s). View

Comments

damiankloip’s picture

Issue summary: View changes

Updated issue summary.

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.phpundefined
@@ -99,15 +99,26 @@ public function set($property_name, $value, $langcode = NULL) {
+   * @var bool $status

@param

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.phpundefined
@@ -99,15 +99,26 @@ public function set($property_name, $value, $langcode = NULL) {
+   * @return \Drupal\Core\Config\Entity\ConfigEntityInterface

Should have a line after it explaining

If we want calling code to be able to rely on enable/disable, they should stay in the interface. So I vote for all three in there.

damiankloip’s picture

FileSize
1.39 KB
1.7 KB
FAILED: [[SimpleTest]]: [MySQL] 49,212 pass(es), 524 fail(s), and 254 exception(s). View

Yeah, I'm not sure about having all of them but I don't mind either way. Here is a new patch that adds setStatus() to the interface.

damiankloip’s picture

Status: Active » Needs review
dawehner’s picture

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.phpundefined
@@ -99,15 +99,24 @@ public function set($property_name, $value, $langcode = NULL) {
+   * @return \Drupal\Core\Config\Entity\ConfigEntityInterface
+   *   The class instance that this method is called on.

This @return should be actually on the interface

damiankloip’s picture

I was thinking that, but does it have to return $this? People might want to implement this slightly differently at some point? not sure.

dawehner’s picture

Well, it should be at least consistent, as you might want to switch out the internal implementation of the class.

Being able to chain stuff is great, so it would makes sense to force it.

damiankloip’s picture

FileSize
1.7 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1911814-7.patch. Unable to apply patch. See the log in the details link for more information. View

Sold! I've moved it to the interface :)

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.phpundefined
@@ -49,6 +49,17 @@ public function enable();
+   * @return \Drupal\Core\Config\Entity\ConfigEntityInterface

Opened #1912544: [policy, then patch] Use @return $this to mark that a method returns itself to make it a bit easier to document what is going on.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x, thanks! Will push once testbot's calmed down a bit. :)

alexpott’s picture

This patch is causing me local fails due to

Exception Recoverabl ConfigEntityBase.  115 Drupal\Core\Config\Entity\ConfigEnt
    Argument 1 passed to Drupal\Core\Config\Entity\ConfigEntityBase::setStatus()
    must be an instance of Drupal\Core\Config\Entity\bool, boolean given, called
    in
    /Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
    on line 109 and
    definedDrupal\Core\Config\Entity\ConfigEntityBase->setStatus()
    Drupal\Core\Config\Entity\ConfigEntityBase->disable()
    Drupal\config\Tests\ConfigEntityStatusTest->testCRUD()
    Drupal\simpletest\TestBase->run()
    simpletest_script_run_one_test('2',
    'Drupal\config\Tests\ConfigEntityStatusTest')

Afaik I know scalar's can not be type hinted so:

  /**
   * Implements \Drupal\Core\Config\Entity\ConfigEntityInterface::setStatus().
   */
  public function setStatus(bool $status) {
    $this->status = $status;
    return $this;
  }

should not have the bool

alexpott’s picture

Status: Fixed » Needs review
FileSize
1.16 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1911814-11.setStatus-method.patch. Unable to apply patch. See the log in the details link for more information. View

Here's a patch to remove the type hinting.

dawehner’s picture

Category: task » bug
Priority: Normal » Critical
FileSize
4.59 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1911814-12.patch. Unable to apply patch. See the log in the details link for more information. View

Please let's also fix the ViewUI class, as it fails pretty hard as well.

Adapting stuff :(

dawehner’s picture

FileSize
1.88 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1911814-13.patch. Unable to apply patch. See the log in the details link for more information. View

Wrong patch.

dawehner’s picture

FileSize
390 bytes
1.87 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1911814-14.patch. Unable to apply patch. See the log in the details link for more information. View

doh

dawehner’s picture

FileSize
551 bytes
1.84 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1911814-15.patch. Unable to apply patch. See the log in the details link for more information. View

Damian pointed out another issue with that.

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
549 bytes
1.88 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1911814-16.patch. Unable to apply patch. See the log in the details link for more information. View

Looks good to me, I have just added casting for the actual $status variable inside the method instead. These tests are passing fine for me locally now.

dawehner’s picture

+1 for that change.

webchick’s picture

Category: bug » task
Priority: Critical » Normal
Status: Reviewed & tested by the community » Fixed

Oops. :\ Sorry, guys.

Committed and pushed to 8.x. Thanks for finding this quickly!!

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.