Expand Block with methods

I'm going to work on this one. :)

As this is my very first issue that I'll ever work on, please be advised, Novice questions/mistakes might arise.

I'll start with an initial patch that'll just convert the weight property to see if I'm on the right track, stay tuned.

Added an initial patch that converts the public weight property to a protected property with a getWeight() and setWeight($weight) accessor methods.

I only modified the weight property at first, just to test the whole flow of making and submitting my patch, as this is my first time doing so :)

The methods are first declared in \Drupal\block\Plugin\Core\Entity\BlockInterface and are documented there as well. I've mirrored this from the Node and NodeInterface classes.

Question: which properties need to be converted? Perhaps it would be a good idea to list each property in the main issue description?

Made the weight property public again because it is accessed dynamically that way by (I think among others) the Config system.

It being protected made this test fail:
Drupal\block\Tests\BlockStorageUnitTest->createTests()

The actual properties that are retrieved via the config for the test block (like this)
$actual_properties = config('block.block.stark.test_block')->get();
don't match up because the weight property couldn't be retrieved.

It should work if you override getExportProperties() and there define the weight. Have a look at how Drupal\views\Plugin\Core\Entity\View does it.

getExportProperties() is already overriden for the Block entity and the 'weight' property is already listed there:

/**
 * Overrides \Drupal\Core\Config\Entity\ConfigEntityBase::getExportProperties();
 */
public function getExportProperties() {
  $properties = parent::getExportProperties();
  $names = array(
    'region',
    'weight',
    'plugin',
    'settings',
    'visibility',
  );
  foreach ($names as $name) {
    $properties[$name] = $this->get($name);
  }
  return $properties;
}

I believe the test in comment #2 failed because of the way properties are retrieved from non-NG entities. As I understand it, whenever a property is accessed on a non-NG entity in a generic way, the get() method from the \Drupal\Core\Config\Entity\ConfigEntityBase class is used. This method is currently implemented as:

/**
 * Overrides Entity::get().
 *
 * EntityInterface::get() implements support for fieldable entities, but
 * configuration entities are not fieldable.
 */
public function get($property_name, $langcode = NULL) {
  // @todo: Add support for translatable properties being not fields.
  return isset($this->{$property_name}) ? $this->{$property_name} : NULL;
}

This implementation makes it impossible to access a property that is protected in a subclass of ConfigEntityBase right? That would break encapsulation.

The original premise of the meta issue was to create accessor methods because doing $entity->property->value() was deemed cumbersome, where $entity->getProperty() is much more elegant.
I think this might only apply for NG entities (on which the work probably started, looking at the meta issue).

Isn't $entity->property even faster/easier than $entity->getProperty()?
And seeing that making the properties protected introduces new issues, (accessing them via the generic get and set system that is shared by all Entities) I'm starting to become unsure if converting properties to accessors should even be done for non-NG entities at all?

Can anyone give me his/her thoughts on this as I'm kind of new to the community and this seems like it's a discussion that concerns the whole (non-NG) Entity architecture?

After talking about this for a bit on IRC with Xano and berdir, this is the summary:
- changing public to protected is an API change, we're past api-freeze at this point so the discussion around this is kind of moot
- the properties will remain public
- we can add extra accessor methods to the Entity Interface and just implement them on the Entity using get and set, this allows for easier type hinting and very easy implementation
- the patch in #5 shows this exactly

I'll add a patch for all (relevant) properties of Block that could use accessors later.

New patch that adds accessor methods to (in my opinion) all relevant properties of the Block entity.

The methods are defined and documented in \Drupal\block\Plugin\Core\Entity\BlockInterface and implemented in \Drupal\block\Plugin\Core\Entity\Block.

Fixed the failing test. I accidentally removed the namespace use statements for the Annotations inside Block.

Re-added them with this patch.

Remove unwanted text

Updated issue summary.

Could you please re-roll this and ill come back and review again.

Rerolled #11.

Removing tags

+++ b/core/modules/block/lib/Drupal/block/BlockInterface.php
@@ -27,4 +27,69 @@
+   *   The visibility restrictions information keyed by type for this block.

This reads a little awkward for me, should it just be "restriction"?

Other than that, +1 for RTBC.

Updated issue summary.

Tagging for reroll.

Unassigning since it's been a while. @Thomas Brekelmans if you still want to work on this please reassign.

Patch does not apply correctly to latest git code base, hence re-rolling patch.

Do we want to look for places the properties are accessed directly and change those to use the new methods? I think we are doing that on some other issues that are part of this meta.

Yes, we should. I checked but could not find in this case. Since properties are set as protected, it makes test fail, where property is accessed directly. I would like somebody to double check it.

Any specific reason to reroll? it is working perfectly, if I missed anything let me know.

No need to reroll at this point, patch applies cleanly

checked this and it needed a reroll. unassigned @Nitesh Sethia since it gone so much time but if you @Nitesh Sethia want to continue just reassign.

Reroll itself was nothing to do, the head was empty.

25: block-entity-weight-property-methods-2030571-25.patch queued for re-testing.

sorry the head was empty but apparently the presave function had changed parameter. Couldn't find any records of that and that changed didn't exist at the head.
hopefully better now.

still needs some work and the presave function puzzles me a little.

What puzzled me was simply that I thought that the presave had been removed but couldn't find any issue for it but finally I found it, #2195753: Changes to config entities that use plugins are not propagated to the plugins. Updated some doc. think we can remove the use statement for Cache too

1. +++ b/core/modules/block/lib/Drupal/block/BlockInterface.php
   @@ -32,4 +32,68 @@
   +   * @return \Drupal\block\BlockInterface
   +   *   The class instance this method is called on.
   ...
   +   * @return \Drupal\block\BlockInterface
   +   *   The class instance this method is called on.
   ...
   +   * @return \Drupal\block\BlockInterface
   +   *   The class instance this method is called on.
   

   Let's use @return $this

2. +++ b/core/modules/block/lib/Drupal/block/BlockInterface.php
   @@ -32,4 +32,68 @@
   +   *   The visibility restrictions information keyed by type for this block.
   

   What is a type of a block?

Since I've not written these comments it's hard to give an appropriate and correct answer but I don't read this as a "type of block", but as the visibility restrictions information is keyed by type for this block

so added an is to the comment so hopefully clearer that it isn't a type of block.

+++ b/core/modules/block/lib/Drupal/block/BlockInterface.php
@@ -32,4 +32,68 @@
+  public function settings();

Why is this method just called "settings" instead of "getSettings"?

Is there a pattern here I'm missing?

Thanks.

ref: https://groups.drupal.org/node/424758 for PSR-4

anyone know the answer to @ericduran question about settings?

@filijonka, @ericduran I think it should be getConfiguration, ref https://drupal.org/developing/api/8/block_api

There is no need for any of these methods since we never use them in code. All of these settings are passed along to the plugin instance and are accessed from there. Lets not add unnecessary code :)

We can just protect all the properties.

Agreed.
But we still have to fix the migrate test.

I can somewhat follow the logic in #38, but are we sure there are no use cases where someone wants to access the properties from the block entity? Or: could someone explain the reasoning behind #38 so I can know whether I can set this to RTBC or not :)

Patch in #40 no longer applies. I will attempt a re-roll.

Here is the patch from #40, only it applies. I made the changes manually - YesCT will learn me to re-roll soon.

The logic is that if the patch passes, then by definition, there are no cases where the properties need to be accessed.

This looks good to me - I've grepped the block module for ->id[^(] and can not find anywhere where we are accessing a block like that.

Per alexpott in IRC, we still need to find where the Block module is using get-> and set-> and replace them with getters and setters for those properties.

Since we will do that in the module, we'll update the migrate tests where they use get-> also (e.g in patch #43)

I made some progress here and per YesCT's advice, I'm posting the patch and hope to come back to it.

1. I'd love feedback on whether I handled the documentation and methods on BlockInterface.php and Block.php correctly. I copied from other sub-issues e.g. https://www.drupal.org/node/2030585.

2. There is still a lot of $entity->get('theme') I am still looking for where the $theme property is defined - I could add the getTheme() and setTheme() methods, but I'd like to know a little more about what I am doing.

3. There is a bunch of ->get('id') ->set('id ... and I do find $entity->id() in BlockViewBuilder.php. Does id really need getters and setters, or it there one in a parent class somewhere?

+++ b/core/modules/block/block.api.php
@@ -154,8 +154,8 @@ function hook_block_view_BASE_BLOCK_ID_alter(array &$build, \Drupal\Core\Block\B
+  if ($operation == 'view' && $block->getPlugin() == 'system_powered_by_block') {

This won't work because getPlugin returns a fully configured plugin object. We need to have getPluginId method that return $this->plugin.

I don't think we need a theme setter because I think we should encapsulate the following functionality from block_theme_initialize() on the block class:

      if (strpos($default_theme_block_id, $default_theme . '_') === 0) {
        $id = str_replace($default_theme, $theme, $default_theme_block_id);
      }
      else {
        $id = $theme . '_' . $default_theme_block_id;
      }
      $block = $default_theme_block->createDuplicate();
      $block->set('id', $id);
      $block->set('theme', $theme);

But we should definitely have a getter for the theme.

$theme isn't defined as a property on the class, so it can't be protected and we don't actually need a getter. But we want to define getTheme() for consistency across all plugins.

In this case, do we want to simply add the property on the class, or else do we need to check if (!empty($this->theme)) on getTheme()?

I think we should encapsulate the following functionality from block_theme_initialize() on the block class:

i.e. in a __construct()?

I declared $theme as a protected property; it is always present on the entity since it is set at initialization.

Test failed because patch no longer applies.

My attempt to re-roll failed. I re-created the patch manually, and caught several instances of direct access to $weight that I had missed previously.

I did not protect $id this time. I see a couple calls to ->id().

I notice some instances of direct access to $status - another undeclared property on Block. Do we want to declare this?

Fixing PHP error.

Another PHP error. Last one I swear. I will check php -l against files in git status from now on.

One more try.

Looks like we shouldn't be declaring $theme after all.

Yep we definitely should be declaring $theme and making it protected. We just have to fix whatever is broken in Drupal\Tests\block\Unit\Plugin\DisplayVariant\FullPageVariantTest

Like so

Final tidy ups to not use ->get($property) or ->set($property, $value) anywhere.

This patch encapsulates the id and theme setting that was done in block_theme_initialise in an override of the createDuplicate() method.

Great. I have a much better sense about what you meant by 'encapsulate' now.

Re-roll of #65.

Missed one... Try again.

I have a few problems with this patch:
- In the class block the variable $id is still public;
- The file FullPageVariant.php is renamed to BlockPageVariant.php in #2352155: Remove HtmlFragment/HtmlPage;
- The file FullPageVariantTest.php must also be renamed;

reclassifying as a bug per the meta and #2030645-31: Expand Menu with methods
also see the meta for evaluation if this can be done at this point in the beta.
Also, the current approach is different than originally, we are making the properties protected and leaving them that way.

removed the added files FullPageVariant/test from the patch
changed the id to be protected

You have made the variables $id and $weight protected. That is the most important. Let the testbot run and see how many failes there are.

+++ b/core/modules/block/src/Entity/Block.php
@@ -114,6 +114,13 @@ class Block extends ConfigEntityBase implements BlockInterface, EntityWithPlugin
   /**
+   * The theme that includes the block plugin for this entity.
+   *
+   * @var string
+   */
+  protected $theme;

Why do you create a new variable named $theme?

+++ b/core/modules/block/src/BlockInterface.php
@@ -33,6 +33,30 @@
+   * Returns the plugin id.
+   *
+   * @return string
+   *   The plugin id for this block.
+   */
+  public function getPluginId();
+
+  /**
+   * Returns the region this block is placed in.
+   *
+   * @return string
+   *   The region this block is placed in.
+   */
+  public function getRegion();
+
+  /**
+   * Returns the theme id.
+   *
+   * @return string
+   *   The theme id for this block instance.
+   */
+  public function getTheme();

@@ -89,4 +113,32 @@
+  /**
+   * Sets the region this block is placed in.
+   *
+   * @param string $region
+   *   The region to place this block in.
+   *
+   * @return $this
+   */
+  public function setRegion($region);
+
+  /**
+   * Sets the block weight.
+   *
+   * @param int $weight
+   *   The desired weight.
+   *
+   * @return $this
+   */
+  public function setWeight($weight);

These function are not necessary. They (almost) never used, so please remove them.

I am not sure about the getWeight() function. Lets see how many times it is needed outside of the testfiles.

@daffie if you look at the interdiff you'll see that those things you're asking is already there. I did check the get* to see if there were any outsides the test for each and there were so. So whoever did the previous patch is the one to ask :)

@daffie all the methods added in this patch have runtime (non test) usages. The theme property is added because it being used without being declared - making this more of a bug fix than some of the other issues.

@filijonka: I know that they are in the previous patch. And the problem is that the person who does the commits does not what getter and setter functions that are (almost) never used. For the cases that you need a getter or a setter use get() or set(). And would like to get this issue fixed, so that is for me the way to go. If you would like to work on it too then please do so. I will do a good review of the posted patches.

Forget my previous post! I will do a review of your patch.

I have reviewed the patch and it all looks good to me.
All the added getter and setter functions are allowed by alexpott.
The class variables are all protected.
The documentation is also in order. I have one documentation issue, see below.
The testbot gives a green result, almost RTBC for me.
If no better solution for the problem below then full RTBC from me.

+++ b/core/modules/block/src/Entity/Block.php
@@ -283,4 +318,39 @@ class Block extends ConfigEntityBase implements BlockInterface, EntityWithPlugin
+  /**
+   * {@inheritdoc}
+   *
+   * @param string $new_id
+   *   (optional) Sets a new id on the duplicate block.
+   * @param null $new_theme
+   *   (optional) Sets a the theme on the duplicate block.
+   */
+  public function createDuplicate($new_id = NULL, $new_theme = NULL) {

I have my doubts about this, but no idea how I can make it better.

you're right about that @daffie, that docblock isn't correct. It would be good but our parser can't make that if I've understood @jhodgdon correct.

So we ned to remove the inheritdoc and write a descriptiopn for this function. It was introduced in #65

Something like this.

/**
   * Creates a duplicate of the entity.
   *
   * @param string $new_id
   *   (optional) Sets a new id on the duplicate block.
   * @param null $new_theme
   *   (optional) Sets a the theme on the duplicate block.
   *
   * @return static
   *   A clone of $this with all identifiers unset, so saving it inserts a new
   *   entity into the storage system.
   */

Copied from core/lib/Drupal/Core/Entity/EntityInterface.php

If you are implementing a member of an interface, then just use {@inheritdoc}, because the interface will be documented.

If you have anything else to add about this specific implementation, add it as // Inline comments.

If your method does something special that the interface isn't documented to do, then U R Doing It Rong. :-)

The problem is that we have a function called createDuplicate(). This function is part of the EntityInterface. In the Block class is this function overridden. Not only is it overridden but there also two parameters added to the function. Our problem is how do you document that. In the current patch is it like in comment #84. It is not very pretty. If you have any idea how we can make this better, please let us know.

See comment #84.

The patch from comment #77 with the change from comment #86 applied.

Talked to filijonka on IRC and we came to the conclusion that we liked the documentation change suggested in comment #86 was better. In command #84 I did a review of the patch made by filijonka and for the documentation problem which we discussed on IRC it was in my eyes RTBC. The only change with the patch in comment #90 is the documentation change. Putting it back to RTBC.

re-roll patch

#94 omitted the 'a' in #86 suggestion. Added the 'a'.

just a quick note, haven't looked at it more than a glance, there are some lines with added spaces that needs to be removed.

@tadityar You can see the whitespace problem that @filijonka mentioned if you have the browser drupal.org plugin: https://dreditor.org/
Also, after your reroll in #94 when you posted the little change in #95, it would have been nice to have an interdiff between 94 and 95. https://www.drupal.org/documentation/git/interdiff

I attached the interdiff, and how I made the interdiff in case that helps.

I'll fix a few things also. new patch (and interdiff) coming.

this just fixes the whitespace and some grammar. it undoes the change in #95 "Sets a the theme on the" a the is does not make sense.

Actually, @param docs are not usually written as actions. "Sets ..." is something a method does, not a parameter. Another patch coming. ... Also, id should be ID. id is http://en.wikipedia.org/wiki/Id,_ego_and_super-ego and ID is identifier.

Another patch coming.

[edit: oops. named my patch 99 instead f 98]

Here is those little fixes in the docs that I mentioned in #98.

1.
@daffie I think this issue needs an issue summary update. Where concerns can be summarized there.

For example...

+++ b/core/modules/block/src/Entity/Block.php
@@ -283,4 +318,43 @@ protected function conditionPluginManager() {
+  /**
+   * Creates a duplicate of the entity.
+   *
+   * @param string $new_id
+   *   (optional) The new ID on the duplicate block.
+   * @param string $new_theme
+   *   (optional) The theme on the duplicate block.
+   *
+   * @return static
+   *   A clone of $this with all identifiers unset, so saving it inserts a new
+   *   entity into the storage system.
+   */
+  public function createDuplicate($new_id = NULL, $new_theme = NULL) {

there are no calls to this method. (that phpstorm finds) Explain that a bit in the summary.

2.
Have the recent children of the meta that make the properties protected been needing change records?

+++ b/core/modules/block/block.module
@@ -159,13 +159,11 @@ function block_theme_initialize($theme) {
+      $block = $default_theme_block->createDuplicate($id, $theme);

1. As you can see in the file block.module is the method called. Also updated the issue summary with the methods that are added.

2. As far as I know there are no change records made for setting properties protected. I think that it is the idea to do that for the parent issue when all its children are done.

Back from #88: "The problem is that we have a function called createDuplicate(). This function is part of the EntityInterface. In the Block class is this function overridden. Not only is it overridden but there also two parameters added to the function. Our problem is how do you document that. In the current patch is it like in comment #84. It is not very pretty. If you have any idea how we can make this better, please let us know."

You don't document, you refactor. If you have extra arguments, you're breaking the interface.

So here's a situation where we want to have a general way to duplicate an entity with createDuplicate(), but we also want to be able to specify things like the block ID and which theme it belongs to. The only thing that happens in this new implementation is that we set these values on the new block object; there aren't any special side-effects that compute anything.

As @YesCT points out, this method doesn't get used very much, and it basically results in two assignments that could happen outside the interface.

So let's not break the interface. We should remove the override of createDuplicate(). We then have two options: We could make a createDuplicateBlock() convenience method, or we could just require that callers set the ID and theme themselves after they create the duplicate. I prefer the latter, since we only ever use our special createDuplicate() override once. To make that happen, we have to add a setTheme(). :-)

No patch here because then I can review it.

I have to agree with Mile23 that changing the parameters of createDuplicate() breaks the interface. But I must say that my preference goes to the createDuplicateBlock() function. It must be added to the BlockInterface. Creating a setTheme() and a setId() function is the alternative.

I found some getters that we can change:

      if ($block->get('plugin') == 'system_menu_block:' . $menu->id()) {

block.module on line 283 in the function block_menu_delete().

      if ($entity->get('region') == BlockInterface::BLOCK_REGION_NONE) {

BlockListBuilder.php on line 405 in the function submitForm().

        $full[$block->get('region')][$block_id] = $block;

BlockRepository.php on line 81 in the function getVisibleBlocksPerRegion().

    $theme = $entity->get('theme');

ConfigTranslationBlockListBuilder.php on line 64 in the function buildRow().

I tried to do an interdiff, but I got a 1 out of 6 hunks FAILED. So I got a partial interdiff. I think that the part about the Block.php file is missing.

@YesCT, thanks for the dreditor suggestion! It makes viewing patches a lot nicer :)
I'll also try to post interdiffs on next patches that I make

I have tried to create an interdiff.txt for 105 to 109, but the interdiff command does not like my 105 patch.

The difference between 105 and 109 is that I have changed the PHPUnit tests in BlockRepositoryTest to use the getRegion() method instead of get('region').

The problem in the MenuCacheTagsTest eludes me.

+++ b/core/modules/block/tests/src/Unit/BlockRepositoryTest.php
@@ -93,12 +93,8 @@ public function testGetVisibleBlocksPerRegion(array $blocks_config, array $expec
       $block->expects($block_config[0] ? $this->atLeastOnce() : $this->never())
-        ->method('get')
-        ->will($this->returnValueMap(array(
-          array('region', $block_config[1]),
-          array('weight', $block_config[2]),
-          array('status', TRUE),
-        )));
+        ->method('getRegion')
+        ->willReturn($block_config[1]);
       $blocks[$block_id] = $block;

My first thought was: Oh no! He's tossing out test data! But then I see that getRegion() will of course be more specific. It might be that the test data could be thinned out by removing whatever's in $block_config[2].

As far as MenuCacheTagsTest is concerned... Let's look near the fail reported by the testbot:

    // Verify a cache hit, but also the presence of the correct cache tags.
    $expected_tags = array(
      'theme:classy',
      'theme_global_settings',
      'rendered',
      'block_view',
      'block:' . $block->id(),
      'block_plugin:system_menu_block__llama',
      'menu:llama',
    );

I see a block or two... This test depends on block module and also test_page_test module, so somewhere in there is llama that needs some getter/setter love.

@Mile23: Thank you for your help. I think that we should make a new issue for the BlockRepositoryTest. The dataprovider is missing a docblock and the implementation can be made more clear.
I will go and work on test_page_test module.

@daffie: We're changing the way block entities work, so it's OK to change the test as long as it's relevant to the refactoring here.

Fixed one bug.

@Mile23: Do you have an issue for

We're changing the way block entities work

? I Would like to get the BlockRepositoryTest improved. And I was wandering if I should make a separate issue for that or not.

1. +++ b/core/modules/block/block.module
   @@ -103,7 +103,7 @@ function _block_rehash($theme = NULL) {
   @@ -114,7 +114,7 @@ function _block_rehash($theme = NULL) {
   

   there's a todo in _block_rehash which as I can see is now something that can be done. Perhaps not really a part of this issue but then a new issue should be done for it

2. +++ b/core/modules/block/block.module
   @@ -159,13 +159,11 @@ function block_theme_initialize($theme) {
   +        $block->setRegion(system_default_region($theme));
   

   just a note, you can't actually in php since long time call a function within another one like this..

3. +++ b/core/modules/block/src/BlockInterface.php
   @@ -89,4 +113,46 @@ public function getContexts();
   +   * @return static
   

   probably wrong about this but strange that a non static function returns a static.

4. +++ b/core/modules/block/src/Entity/Block.php
   @@ -144,6 +151,34 @@ public function getPluginCollections() {
       * Overrides \Drupal\Core\Entity\Entity::label();
   

   should be inheritdoc

5. +++ b/core/modules/block/src/Entity/Block.php
   @@ -162,13 +197,13 @@ public function label() {
      public static function sort(ConfigEntityInterface $a, ConfigEntityInterface $b) {
   

   the docblock of this need to be correct written with @param etc

6. +++ b/core/modules/block/src/Tests/BlockStorageUnitTest.php
   @@ -120,9 +120,9 @@ protected function loadTests() {
   +    $this->assertTrue($entity->status());
   

   not a part of this issue but strange that we have a setStatus and then just status and not getStatus..

Not going to change the status of this issue because all comments above can be discussed if they are really part of this issue.

@filijonka: Thank you for your review. They are all good points only not relevant to this issue. You can create new issues for some of your points.

4 is actually valid.

@filijonka: With point 4 you are right. It needs to be changed to inheritdoc for the function Block::label(). But in my eyes that is not part of this issue.

with valid I meant valid for this issue.

I was a bit quick. A small documentation change is allowed. I think. I shall make a new patch.

Minor documentation change. Good find filijonka.

hmm maybe I was wrong about 117:4 to add that in this but let committer descide that.
117:5 should be as simply, just an inheritdoc.

looks good otherwise.

Committed/pushed to 8.0.x, thanks!