ShortcutSet class variables should not be accessed directly. Functions should be use to access the variable. For instance use getDescription() and setDescription($description) for the protected class variable description. For a boolean variable the getter function becomes isVariableName(). In object-oriented programming this is called encapsulation.

Remaining tasks

  • Update the class variables and make them protected.
  • Create getters and setters for frequently used get and set functionality.
  • Update drupal to use the getters and setters instead of accessing variables directly.
  • There are no tests required because the added functions are only getters and setters.

For more info over what should be done see the issue summary of #2016679: Expand Entity Type interfaces to provide methods, protect the properties.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because properties should not be public, API methods should not be allowed to be sidestepped.
Issue priority Major because this meta goes across the entire system. But each child will be a normal bug.
Prioritized changes Prioritized since it is a bug and it reduces fragility.
Disruption Somewhat disruptive for core as well as contributed and custom modules:
  • BC break for anything using the public properties: code will need to convert to the methods
  • BC break for anything (mis)using properties that should not really be public: will require minor refactoring
  • BC break for alternate implementations of a given entity interface (rare/probably nonexistent): they will need to implement the new methods

But impact will be greater than the disruption, so it is allowed in the beta.

CommentFileSizeAuthor
#23 drupal-2384527-22.patch1.1 KBpreshetin
#21 drupal-protecting-2384527-21.patch39.48 KBtadityar
#15 drupal-protecting-2384527.patch1.09 KBtadityar
#12 2384527-11.patch581 bytesrpayanm
#4 protect-2384527-4.patch581 bytesareke

Comments

daffie’s picture

daffie commented

I would like to get this fixed. So I will do a good review for posted patches.

daffie’s picture

daffie commented
Component: node system » shortcut.module
daffie’s picture

daffie commented
Issue summary: View changes

The class variables $id and $label need to become protected.
No extra functions need to be added.

areke’s picture

areke commented
StatusFileSize
new protect-2384527-4.patch581 bytes

A patch to protect $id and $label.

areke’s picture

areke commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 4: protect-2384527-4.patch, failed testing.

daffie’s picture

daffie commented

The reason that your patch failed testing is that outside the NodeType class the class variables are no longer accessible. And it will result in an error. Use the methods id() and label() to fix this. If you need the set a variable use set('id', $value) or set('label', $value).

areke’s picture

areke commented
Assigned: Unassigned » areke
rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented

any progress @areke?

areke’s picture

areke commented
Assigned: areke » Unassigned

@rpayanm, not yet. If you want you can take it.

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Assigned: Unassigned » rpayanm

Working on this!

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Status: Needs work » Needs review
StatusFileSize
new 2384527-11.patch581 bytes

Let me see.

Status: Needs review » Needs work

The last submitted patch, 12: 2384527-11.patch, failed testing.

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Assigned: rpayanm » Unassigned
tadityar’s picture

tadityar commented
Assigned: Unassigned » tadityar
Status: Needs work » Needs review
StatusFileSize
new drupal-protecting-2384527.patch1.09 KB

Trying.

Status: Needs review » Needs work

The last submitted patch, 15: drupal-protecting-2384527.patch, failed testing.

tadityar’s picture

tadityar commented
Assigned: tadityar » Unassigned
daffie’s picture

daffie commented

The problem is that the ShortcutSetsTest.php is failing. Seen as we changed the class variables from public to protected they are no longer directly accessible outside of the class ShortcutSet. Since the failing test is part of the same module shortcut as is the class. I would start by looking for places in the module where a shortcut entity is accessing the variable $id or $label directly (->id or ->label) and not via a method (->id() or ->label()). Hope this will help.

tadityar’s picture

tadityar commented

@daffie is this correct though changing the
$default_set()->id() to $default_set('id', $value)?

Thank you for your explanation, gonna try again :)

daffie’s picture

daffie commented 
+++ b/core/modules/shortcut/src/Entity/ShortcutSet.php
@@ -68,7 +68,7 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
       $default_set = shortcut_default_set();
       // This is the default set, do not copy shortcuts.
-      if ($default_set->id() != $this->id()) {
+      if ($default_set('id', $value) != $this->id()) {
...
 

The function shortcut_default_set() returns an instance of ShortcutSet. If you want to do something with it you must use methods. If you want to set the value of the variable $id do $default_set->set('id', your_new_id_value).

tadityar’s picture

tadityar commented
Status: Needs work » Needs review
StatusFileSize
new drupal-protecting-2384527-21.patch39.48 KB

Status: Needs review » Needs work

The last submitted patch, 21: drupal-protecting-2384527-21.patch, failed testing.

preshetin’s picture

preshetin commented
Assigned: Unassigned » preshetin
Status: Needs work » Needs review
StatusFileSize
new drupal-2384527-22.patch1.1 KB

Trying. In this patch I tried to implement #18 instructions.

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

All the class variables are protected.
There are already getter functions available, so no need for new ones.
The test-server give it green.
It all looks good to me, so for me it is RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 6031d0b and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation for to the issue summary.

diff --git a/core/modules/shortcut/shortcut.module b/core/modules/shortcut/shortcut.module
old mode 100644
new mode 100755

diff --git a/core/modules/shortcut/src/Entity/ShortcutSet.php b/core/modules/shortcut/src/Entity/ShortcutSet.php
old mode 100644
new mode 100755

We should not be changing the file modes to 755. Fixed on commit.

  • alexpott committed 6031d0b on 8.0.x 
    Issue #2384527 by tadityar, areke, rpayanm, preshetin: Make the class...

Status: Fixed » Closed (fixed)

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