Clean up edit controller [#2033433]

Problem/Motivation

@dawehner noted several points of improvement to the code committed in #1901100: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits.

Just a couple of points I realized when reviewing the patch.

+++ b/core/modules/edit/lib/Drupal/edit/Access/EditEntityAccessCheck.phpundefined
@@ -0,0 +1,67 @@
+    return $entity->access('update');
Access checkers should return static::ALLOW and static::DENY.

+++ b/core/modules/edit/lib/Drupal/edit/Access/EditEntityAccessCheck.phpundefined
@@ -0,0 +1,67 @@
+      if (!$entity_type || !entity_get_info($entity_type)) {
...
+      $entity = entity_load($entity_type, $entity_id);

This information should be better injected.

+++ b/core/modules/edit/lib/Drupal/edit/Access/EditEntityAccessCheckInterface.phpundefined
@@ -0,0 +1,22 @@
+interface EditEntityAccessCheckInterface {

It is hard to see why this need to be a new interface? There is no single place in core where this is called, beside of the only class which implements it.

+++ b/core/modules/edit/lib/Drupal/edit/EditController.phpundefined
@@ -24,6 +27,23 @@
+  public function __construct(TempStoreFactory $temp_store_factory = NULL) {
+    $this->tempStoreFactory = $temp_store_factory ?: Drupal::service('user.tempstore');
+  }

I don't get why we even need lazy-injection, can't we just implement ControllerInterface and inject the dependency?

+++ b/core/modules/edit/lib/Drupal/edit/EditController.phpundefined
@@ -110,17 +130,27 @@ public function attachments(Request $request) {
+    if ($tempstore_entity && !(isset($_POST['reset']) && $_POST['reset'] === 'true')) {

$_POST could be easily replaced by using the information on the $request object.

Proposed resolution

Make the code better.

Remaining tasks

Address @dawehner's concerns.

User interface changes

None.

API changes

None.

#1901100: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits

Comment	File	Size	Author
#10	edit_controller_cleanup-2033433-10.patch	10.14 KB	Wim Leers
#10
#10	interdiff.txt	2.54 KB	Wim Leers
#7	edit_controller_cleanup-2033433-7.patch	8.82 KB	Wim Leers
#7
#7	interdiff.txt	3.58 KB	Wim Leers
#2	edit_controller_cleanup-2033433-2.patch	7.28 KB	Wim Leers
#2

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 2 July 2013 at 21:08

Access checkers should return static::ALLOW and static::DENY.

That is new then, we were following the older "latest best practices".

This information should be better injected.

Similarly.

It is hard to see why this need to be a new interface? There is no single place in core where this is called, beside of the only class which implements it.

True, I think Gábor did this mostly because that's what everything else is doing. Note that if parameter upcasting wasn't so terribly broken, we wouldn't need any of this at all! :(

I don't get why we even need lazy-injection, can't we just implement ControllerInterface and inject the dependency?

Great point — this slipped through the cracks!

$_POST could be easily replaced by using the information on the $request object.

Indeed! But at some point something was broken in $request, which caused me to just use $_POST instead until that got fixed.

We'll get it fixed :)

Comment #2

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 17 July 2013 at 14:51

Assigned:	Unassigned	» Wim Leers
Status:	Active	» Needs review

File	Size
edit_controller_cleanup-2033433-2.patch	7.28 KB

This information should be better injected.

I'm actually not sure what this means. Can you point to a code example that's doing it "right"?

Attached patch fixes everything except the above.

Comment #3

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 17 July 2013 at 15:07

I think this looks like a good update! I am also puzzled as to what should we do for entity injection(?).

Comment #4

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 18 July 2013 at 08:16

I think what is meant that instead of entity_get_info($entity_type) you should be calling Drupal::entityManager()->getDefinition($entity_type) and instead of entity_load($entity_type, $entity_id) you should be doing Drupal::entityManager()->getStorageController($entity_type)->load($entity_id) . Probably save Drupal::entityManager() in a local variable to save some keystrokes as well...

Comment #5

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 18 July 2013 at 08:51

Ok, injecting the entity manager sounds sensible, yup :)

Comment #6

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 18 July 2013 at 09:05

Status:

Needs review

» Needs work

#4: lovely, thanks :)

Comment #7

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 18 July 2013 at 13:17

Status:

Needs work

» Needs review

File	Size
interdiff.txt	3.58 KB
edit_controller_cleanup-2033433-7.patch	8.82 KB

Finished the last bit that I didn't address yet in #2, thanks to @tstoeckler's guidance in #4. I went a bit further, and am now also injecting the field.info service, so the call to field_info_instance() (which is deprecated in D8) is now also gone!

Comment #8

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 18 July 2013 at 18:23

+++ b/core/modules/edit/lib/Drupal/edit/Access/EditEntityAccessCheck.php
@@ -42,7 +42,7 @@ public function access(Route $route, Request $request) {
   public function accessEditEntity(EntityInterface $entity) {

I think this function should be protected.

+++ /dev/null
index 9ec569a..90313f9 100644
--- a/core/modules/edit/lib/Drupal/edit/Access/EditEntityFieldAccessCheck.php

--- a/core/modules/edit/lib/Drupal/edit/Access/EditEntityFieldAccessCheck.php
+++ b/core/modules/edit/lib/Drupal/edit/Access/EditEntityFieldAccessCheck.php

I think EditEntityFieldAccessCheck should use the same approach that EditEntityAccessCheck uses, i.e. no separate interface, protected method, static::ALLOW / static::DENY.

+++ b/core/modules/edit/lib/Drupal/edit/Access/EditEntityAccessCheck.php
@@ -42,7 +42,7 @@ public function access(Route $route, Request $request) {
   public function accessEditEntity(EntityInterface $entity) {

I think this function should be protected.

Comment #9

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 18 July 2013 at 20:14

Status:

Needs review

» Needs work

Comment #10

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 19 July 2013 at 18:24

Status:

Needs work

» Needs review

File	Size
interdiff.txt	2.54 KB
edit_controller_cleanup-2033433-10.patch	10.14 KB

I think this might be the RTBC one.

Comment #11

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 22 July 2013 at 15:15

Status:

Needs review

» Reviewed & tested by the community

This looks good to me!

Comment #12

catch

he/him

English

CreditAttribution: catch commented 24 July 2013 at 08:50

Title:	Issue #1901100 does not follow best practices in several places in the committed patch	» Clean up edit controller
Status:	Reviewed & tested by the community	» Fixed

Committed/pushed to 8.x, thanks!