Expand BlockContent and BlockContentType with methods

I'll take this one on as well :)

Only the public $description property was left for CustomBlockType. All other properties have accessor methods for them defined by the base classes ConfigEntity and/or Entity, right?

So this patch adds a getDescription() and setDescription($description) method. :)

I've updated the patch to also add methods for properties on the CustomBlock entity.

I only added a getter for the custom block type as that is something that shouldn't change on the fly by anyone right?

Updated issue summary.

Updated issue summary.

Please add interdiffs to follow-up patches. See https://drupal.org/documentation/git/interdiff about the why and how of interdiffs.

Here's an interdiff for the patch in #4 compared to the patch in #3. :)

@@ -231,4 +231,40 @@ public function delete() {
+    return $this->log;

How come the property is info rather than description? Seems inconsistent.

Other than this looks good.

1. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockInterface.php
   @@ -46,4 +46,49 @@ public function getTheme();
   +   * @param string
   +   *   The desired description.
   
   +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockTypeInterface.php
   @@ -13,5 +13,22 @@
   +   * @param string
   +   *   The desired description.
   

   @param are missing the variable name, like @param string $description.

2. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Core/Entity/CustomBlock.php
   @@ -231,4 +231,40 @@ public function delete() {
   +    return $this->type;
   ...
   +    return $this->info;
   

   This is a content entity, so should be more something like $this->get('type')->value.

   We should probably convert calls to these methods to make sure that they actually work.

Rerolling #4

Using $this->entityType() in getCustomBlockType() method in response to #8.

Updated issue summary.

10: custom-block-entity-expand-custom-block-with-methods-2030597-9.patch queued for re-testing.

maybe this?

@larowlan do you think it is a good idea to add some more assertions to an existing test so that we can test these methods.

@jibran - yes, or a new phpunit test?

@larowlan: I did a review of your patch and it looking good.

As far as I know the drupal way at this moment for get and set functions is:

/**
  * {@inheritdoc}
  */
 public function getLogMessage() {
   return $this->get('log');
 }

 /**
  * {@inheritdoc}
  */
 public function setLogMessage($log) {
   $this->set('log', $log);
   return $this;
 }

For adding a new PHPUnit test.
Some of the sub-issues of #2016679: Expand Entity Type interfaces to provide methods, protect the properties have PHPUnit tests and some have not.

See #8.2, this is still missing the ->value after $this->get('info') and similar.

Here we go.. applying the changes suggested in #8.2 (#23).

Looks like a random fail..queuing for re-test.

24: drupal-custom_block-expand-methods-2030597-24.patch queued for re-testing.

Here we go. The patch passed the tests.
Setting back to needs review.

There is an issue with the file paths in PSR4--I'm currently checking the patch to see what needs to be fixed to make this work...

Tagging.

This is not failing but still needs reroll. It is a novice task since you're totally allowed to ask the mentor about the implications/reason of PSR-4 and not go into those too deeply yourself. It comes down to this:

replace lib/Drupal/custom_block/ by src/ in the paths.

#Amsterdamsprint2014

I am starting to work on this.

I've rerolled your patch without the services.yaml file. You accidentally included that one. Now cleanly applies.

Thank you basvanderheijden.
I also want to add something; custom_block was renamed to content_block. This means the patch consists of more changes as specified at #30

It is renamed to block_content. Not content_block.

In the class BlockContentType the variables $id and $label do not have to be declared. They are inherited from the class Entity.

The $revision and $description are public and must be changed to protected. The whole idea of this issue is to use encapsulation.

The patch is 2 months old so it needs a reroll.

The function getCustomBlockType() must be changed to getBlockContentType().

The method is already getBlockContentType, just the issue title was wrong.
Fixed description and revision, added new methods for revision - made them consistent with NodeType.
$id and $label aren't in any of the base-classes for the config-entity - that's only for content entities.
Fixed some CS issues in previous patch.

New patch with re-roll

Fixed some fails from changes at #40 but also found a fail in BlockContent::getBlockContentType, there is no ->value in ER field.

I have reviewed your patch and I give it a RTBC. All entity variables are protected. The new functions are added to their interfaces. And they are documented. I am happy.

You also changed some comments from "custom block" to "block content". It is fine by me. Only if I do a search in the module directory for the word "custom" I get 139 results. Should we make a new issue for this? There is a #2062715: [META] Many UI/UX issues with custom blocks.

1. +++ b/core/modules/block_content/src/BlockContentTypeInterface.php
   @@ -14,4 +14,42 @@
   +  public function isNewRevision();
   ...
   +  /**
   +   * Set whether a new revision should be created by default.
   +   *
   +   * @param bool $new_revision
   +   *   TRUE if a new revision should be created by default.
   +   *
   +   * @return \Drupal\block_content\BlockContentTypeInterface
   +   *   The class instance this method is called on.
   +   */
   +  public function setNewRevision($new_revision);
   

   Never used.

2. +++ b/core/modules/block_content/src/BlockContentInterface.php
   @@ -80,4 +80,50 @@ public function getTheme();
   +  /**
   +   * Sets the description of the block to the given value.
   +   *
   +   * @param string $description
   +   *   The desired description.
   +   *
   +   * @return \Drupal\block_content\BlockContentInterface
   +   *   The class instance this method is called on.
   +   */
   +  public function setDescription($description);
   
   +++ b/core/modules/block_content/src/BlockContentTypeInterface.php
   @@ -14,4 +14,42 @@
   +  /**
   +   * Sets the description of the block type to the given value.
   +   *
   +   * @param string $description
   +   *   The desired description.
   +   *
   +   * @return \Drupal\block_content\BlockContentTypeInterface
   +   *   The class instance this method is called on.
   +   */
   +  public function setDescription($description);
   
   +++ b/core/modules/block_content/src/Entity/BlockContent.php
   @@ -137,6 +137,43 @@ public function delete() {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setDescription($description) {
   +    $this->set('info', $description);
   +    return $this;
   +  }
   

   Never used.

3. +++ b/core/modules/block_content/src/BlockContentInterface.php
   @@ -80,4 +80,50 @@ public function getTheme();
   +  /**
   +   * Returns the block revision log message.
   +   *
   +   * @return string
   +   *   The log message for the current revision of this block.
   +   */
   +  public function getLogMessage();
   ...
   +  /**
   +   * Sets the the block revision log message to the given value.
   +   *
   +   * @param string $log
   +   *   The desired log message for the current revision of this block.
   +   *
   +   * @return \Drupal\block_content\BlockContentInterface
   +   *   The class instance this method is called on.
   +   */
   +  public function setLogMessage($log);
   
   +++ b/core/modules/block_content/src/Entity/BlockContent.php
   @@ -137,6 +137,43 @@ public function delete() {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getLogMessage() {
   +    return $this->get('log')->value;
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setLogMessage($log) {
   +    $this->set('log', $log);
   +    return $this;
   +  }
   

   What is wrong with setRevisionLog and getRevisionLog that are already on the interface. These are unused and should be removed.

@alexpott: I noticed your "Not used" argument in a few of these issues now, but I'm a bit confused by that. In all those patches that I wrote, "being used or not" was never a relevant criteria for me. We're just seeing core, who knows what contrib will do. "Being a public API or not" should be the criteria IMHO, and things like setting the description in my opinion are?

Bravely done what alexpott suggested.

@alexpott: Can you respond to Berdirs comment #45.

Patch does not apply.

Trying to reroll

// patch number typo, sorry for this

Good work the_contributer. With some work we shall get it fixed!

+++ b/core/modules/block_content/src/BlockContentInterface.php
@@ -80,4 +80,20 @@ public function getTheme();
+  /**
+   * Returns the block description.
+   *
+   * @return string
+   *   The description of this block.
+   */
+  public function getDescription();

+++ b/core/modules/block_content/src/Entity/BlockContent.php
@@ -137,6 +137,20 @@ public function delete() {
+  /**
+   * {@inheritdoc}
+   */
+  public function getDescription() {
+    return $this->get('info')->value;
+  }

The BlockContent class does not need this function. It is not used.

+++ b/core/modules/block_content/src/Tests/BlockContentRevisionsTest.php
@@ -45,7 +45,7 @@ protected function setUp() {
-      $block->setNewRevision(TRUE);
+      $block->set('revision', TRUE);

@@ -78,7 +78,7 @@ public function testRevisions() {
-    $loaded->setNewRevision();
+    $loaded->set('revision', FALSE);

This is the entity Block. Not BlockContent or BlockContentType.

+++ b/core/modules/block_content/src/BlockContentForm.php
@@ -8,6 +8,7 @@
+use Drupal\block_content\Entity\BlockContentType;

@@ -76,18 +77,17 @@ public static function create(ContainerInterface $container) {
-    $block_type = entity_load('block_content_type', $block->bundle());
...
-    $block->setNewRevision($block_type->revision);
+    $block->set('revision', BlockContentType::load($block->getBlockContentType())->isNewRevision());

This change is not necessary.

Thanks for the corrections.

As for BlockContentForm.php. If we leave $block->setNewRevision($block_type->revision); unchanged, then should we unprotect $revision back? (or use ->isNewRevision()) Else we get a permission error.

Also after last updates the 'revision' field is unknown to $block->set. So I return it to $block->setNewRevision();

Eh. Seems I forgot to change the seven.theme in patch.

Good work adci_contributor. Almost there. I got two minor issues for you left.

+++ b/core/modules/block_content/src/Entity/BlockContentType.php
@@ -47,27 +47,41 @@ class BlockContentType extends ConfigEntityBundleBase implements BlockContentTyp
+  ¶

This line generates a warning: 1 line adds whitespace errors.

+++ b/core/modules/block_content/src/BlockContentInterface.php
@@ -80,4 +80,12 @@ public function getTheme();
+  public function getBlockContentType();

+++ b/core/modules/block_content/src/Entity/BlockContent.php
@@ -137,6 +137,13 @@ public function delete() {
+  public function getBlockContentType() {

I think that it is better to rename this function to getType(). The entity Node has also a getType() method to get the bundle. It is part of the BlockContent entity. So it is kind of obvious that if call getType() that you get getBlockContentType().

Thanks again!

Only one nitpick left. After that it is for me RTBC.

+++ b/core/modules/block_content/src/BlockContentTypeInterface.php
@@ -14,4 +14,21 @@
+
+

Nitpick: There are now two blank lines after the function isNewRevision().

cleaned up

@adci_contributor: Please add an interdiff.txt to your patch. It makes review live a lot easier. I have added one for your last patch.

It all looks good to me.
The patch fixes the problem as described in the issue summary.
There is one new method added to the class BlockContent: getType().
There are two new methods added to the class BlockContentType: getDescription() and isNewRevision().
All documentation is in order.
The patch gets an RTBC from me.

1. +++ b/core/modules/block_content/src/BlockContentForm.php
   @@ -85,7 +85,7 @@ protected function prepareEntity() {
   -    $block->setNewRevision($block_type->revision);
   +    $block->setNewRevision($block_type->isNewRevision());
   
   +++ b/core/modules/block_content/src/BlockContentTypeInterface.php
   @@ -14,4 +14,20 @@
   +  /**
   +   * Returns whether a new revision should be created by default.
   +   *
   +   * @return bool
   +   *   TRUE if a new revision should be created by default.
   +   */
   +  public function isNewRevision();
   
   +++ b/core/modules/block_content/src/Entity/BlockContentType.php
   @@ -47,27 +47,41 @@ class BlockContentType extends ConfigEntityBundleBase implements BlockContentTyp
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function isNewRevision() {
   +    return $this->revision;
   +  }
   

   This method name is misleading since the existing content entity isNewRevision() means something different. The revision property is about whether or not to create new revisions of the BlockContent entities not about whether the we are dealing with a new revision of the BlockContentType entity. We should rename this method to reflect this - perhaps something like createNewBlockContentRevision(). Also the documentation needs to clarify that this determines whether we create new revision of BlockContent entities.

2. +++ b/core/modules/block_content/src/BlockContentInterface.php
   @@ -80,4 +80,12 @@ public function getTheme();
   +  /**
   +   * Returns the block content type (bundle) name.
   +   *
   +   * @return string
   +   *   The name of the block content type (bundle).
   +   */
   +  public function getType();
   
   +++ b/core/modules/block_content/src/Entity/BlockContent.php
   @@ -143,6 +143,13 @@ public function delete() {
      /**
       * {@inheritdoc}
       */
   +  public function getType() {
   +    return $this->bundle();
   +  }
   

   This is not used and unnecessary - just use the bundle() method.

We have getType() as well for Node, for example.

NodeTypeInterface also uses isNewRevision() for this property...

@Berdir ok I can buy that - it think the getType() method is a shame. bundle() matches the key and plenty of methods and getType() is just match the arbitrary name of the bundles. Not to mention the confusion with entity type. But we have precedence so fair enough.

Yeah, consistently bad ;)

Not completely sure myself.. there is an issue open that NodeInterface::getType() is bad, suggesting getTypeId(), but the thing is that the id property of node type is type :) That's not the case for block content though.

I'm not sure that consistency with node is argument enough, just wanted to point it out :)

Okay because the more I think about it the more I'm inclined to think "two wrongs don;t make a right" and stick by #68. Bad names in the node module does not mean we have to have bad names in the block_content module.

Talked to @alexpott on IRC and the decision is made to change the name of isNewRevision() to shouldCreateNewRevision().

Todo from the patch from comment #66:

1. Remove the method BlockContent::getType() and use bundle().
2. Rename the method BlockContentType::isNewRevision() to BlockContentType::shouldCreateNewRevision().

Updated the patch from comment #66 with the instructions from #73.
Sorry. Ignore it.

Updated the patch from comment #66 with the instructions from #73.

It all looks good to me.
All the changes that @alexpott wanted are in the patch.
The patch fixes the problem as described in the issue summary.
There are two new methods added to the class BlockContentType: getDescription() and shouldCreateNewRevision().
All documentation is in order.
The patch gets an RTBC from me.

Hi @adci_contributor can you confirm that this account is the work of a single person rather than a shared account. As per Drupal's terms of service (https://www.drupal.org/terms) - see section A. I sent you an email via your contact form last week and I have not had a response yet. The lack of reply leads me to think that this account is a shared account.

+1 RTBC. Although the title mentions BlockContent, looking at HEAD it is already done

Hello, @alexpott
Yes, I confirm that I'm the only one who uses this account.
I didn't check emails as I was on vacation. I see your email now. Would you like me to respond it?
Thank you for accepting my patch. I hope, my account name won't confuse you again.

@adci_contributor thanks for responding.

Beta evaluation is in the meta. Committed a549c01 and pushed to 8.0.x. Thanks!