For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.

Part of #1971384: [META] Convert page callbacks to controllers

Files: 
CommentFileSizeAuthor
#61 1974376-aggregator-categories-controller-61.patch11.4 KBstella
PASSED: [[SimpleTest]]: [MySQL] 58,127 pass(es).
[ View ]
#61 1974376-55-61-interdiff.txt1.01 KBstella
#60 1974376-aggregator-categories-controller-60.patch9.78 KBstella
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#60 1974376-55-60-interdiff.txt6.71 KBstella
#55 1974376-aggregator-categories-controller-55.patch11.38 KBcbiggins
FAILED: [[SimpleTest]]: [MySQL] 57,786 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#55 1974376-49-55-interdiff.txt427 bytescbiggins
#50 1974376-aggregator-categories-controller-49.patch11.4 KBcbiggins
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#50 1974376-44-49-interdiff.txt2.54 KBcbiggins
#49 1974376-aggregator-categories-controller-49.patch11.5 KBpwieck
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#44 1974376-aggregator-categories-controller-44.patch11.38 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 57,659 pass(es).
[ View ]
#44 interdiff.txt4.72 KBkim.pepper
#42 1974376-aggregator-categories-controller-42.patch7.53 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,052 pass(es).
[ View ]
#40 1974376-aggregator-categories-controller-40.patch7.82 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,616 pass(es).
[ View ]
#37 1974376-aggregator-categories-controller-36.patch7.97 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,763 pass(es).
[ View ]
#35 1974376-aggregator-categories-controller-35.patch9.84 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 56,009 pass(es).
[ View ]
#32 1974376-aggregator-categories-controller-32.patch9.49 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,622 pass(es).
[ View ]
#32 interdiff.txt2.65 KBkim.pepper
#30 1974376-aggregator-categories-controller-30.patch9.09 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,596 pass(es).
[ View ]
#30 interdiff.txt1.02 KBkim.pepper
#28 1974376-aggregator-categories-controller-28.patch9.07 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,526 pass(es).
[ View ]
#28 interdiff.txt1.2 KBkim.pepper
#22 1974376-aggregator-categories-controller-22.patch9.23 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 55,606 pass(es), 58 fail(s), and 23 exception(s).
[ View ]
#22 interdiff.txt1.3 KBkim.pepper
#11 1974376-aggregator-categories-controller-11.patch9.15 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,943 pass(es).
[ View ]
#11 interdiff.txt1.3 KBkim.pepper
#10 1974376-aggregator-categories-controller-10.patch9.15 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,650 pass(es).
[ View ]
#10 interdiff.txt1.77 KBkim.pepper
#7 1974376-aggregator-categories-controller-7.patch9.07 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,794 pass(es).
[ View ]
#7 interdiff.txt639 byteskim.pepper
#5 1974376-aggregator-categories-controller-5.patch9.08 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,535 pass(es).
[ View ]
#5 interdiff.txt3.33 KBkim.pepper
#4 1974376-aggregator-categories-controller-4.patch6.44 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,308 pass(es).
[ View ]
#4 interdiff.txt3.57 KBkim.pepper
#1 1974376-aggregator-categories-controller-1.patch4.63 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,455 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new4.63 KB
PASSED: [[SimpleTest]]: [MySQL] 55,455 pass(es).
[ View ]

As discussed with @berdir in IRC, creating an implementation of AccessCheckInterface to check whether any categories exists seems like overkill for this conversion.

As a result, the 'Categories' menu item now appears, even if there are no categories.

Assigned:Unassigned» kim.pepper

Status:Needs review» Needs work

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -56,4 +56,41 @@ public function feedAdd() {
+    $result = db_query('SELECT c.cid, c.title, c.description FROM {aggregator_category} c LEFT JOIN {aggregator_category_item} ci ON c.cid = ci.cid LEFT JOIN {aggregator_item} i ON ci.iid = i.iid GROUP BY c.cid, c.title, c.description');

this should not be db_query bit $this->database->query. You need to inhect it,like the entity manager

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -56,4 +56,41 @@ public function feedAdd() {
+    $aggregator_summary_items = config('aggregator.settings')->get('source.list_max');

same with this. you need to inject config.factory service

As a result, the 'Categories' menu item now appears, even if there are no categories.

Thats bad. I would like to convert it to an AccessCheckInterface cause when #15266: Replace aggregator category system with taxonomy goes in, it will be possible for anyone to just delete the category field..we need a way to tell if we should show a menu link or not

Status:Needs work» Needs review
StatusFileSize
new3.57 KB
new6.44 KB
PASSED: [[SimpleTest]]: [MySQL] 55,308 pass(es).
[ View ]

Injecting all the dependencies.

Still to create an AccessCheck

StatusFileSize
new3.33 KB
new9.08 KB
PASSED: [[SimpleTest]]: [MySQL] 55,535 pass(es).
[ View ]

Now with accesscheck implementation.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -56,4 +82,41 @@ public function feedAdd() {
+   * @return string
+   *   An HTML formatted string.
...
+    return $build;

It seems to return a build array instead of a render list.

StatusFileSize
new639 bytes
new9.07 KB
PASSED: [[SimpleTest]]: [MySQL] 55,794 pass(es).
[ View ]

Oops! That was copied and pasted from the original function.

Updated comment.

+++ b/core/modules/aggregator/aggregator.routing.ymlundefined
@@ -25,3 +25,10 @@ aggregator_feed_add:
+  requirements:
+    _access_aggregator_categories: 'TRUE'
+++ b/core/modules/aggregator/lib/Drupal/aggregator/Access/CategoriesAccessCheck.phpundefined
@@ -0,0 +1,50 @@
+    // @todo Replace user_access() with a correctly injected and session-using
+    // alternative.
+    return user_access('access news feeds') && (bool) $this->database->queryRange('SELECT 1 FROM {aggregator_category}', 0, 1)->fetchField();

I think you can combine multiple access tags and don't have to worry about the permission in here but just the query.

I hoped we could get rid of this check as you can also just disable the menu link if you don't want it. But as this is straight conversion, let's keep it for now. Maybe we can do that when that thing is a view, then you can just disable the view...

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Access/CategoriesAccessCheck.phpundefined
@@ -0,0 +1,50 @@
+   * Database Service Object.
+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -24,20 +26,44 @@ class AggregatorController implements ControllerInterface {
+   * The database connection.

Let's make these two comments consistent.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Access/CategoriesAccessCheck.phpundefined
@@ -0,0 +1,50 @@
+   * Constructs a CategoriesAccessCheck object.
+   */
+  public function __construct(Connection $database) {

Needs docs.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -56,4 +82,41 @@ public function feedAdd() {
+    // TODO move included functions to controller.
+    module_load_include('inc', 'aggregator', 'aggregator.pages');

I'm not sure whether put helpful helper methods always into the controller is the best thing you can do, because it removes reusability. Additional the code style standard is "@todo" see http://drupal.org/node/1354

StatusFileSize
new1.77 KB
new9.15 KB
PASSED: [[SimpleTest]]: [MySQL] 55,650 pass(es).
[ View ]

Thanks for the review @dawehner. This fixes issues in #9.

StatusFileSize
new1.3 KB
new9.15 KB
PASSED: [[SimpleTest]]: [MySQL] 55,943 pass(es).
[ View ]

Oops! Sorry @berdir. Missed your comments. The following moves the access aggregator feeds check back to the router.

+++ b/core/modules/aggregator/aggregator.routing.ymlundefined
@@ -32,3 +32,11 @@ aggregator_opml_add:
+    permissions: 'access news feeds'
+    _access_aggregator_categories: 'TRUE'

I think that would need to be _permission, no?

Also, I noticed, again, that this currently doesn't work.

I don't understand why PermissionAccessCheck is implemented as it is, that just makes no sense to me.

Cases like this seems to be the 80% use case, where you have multiple conditions and each must be true. Right now, the permission access check only grants and doesn't deny access. (Access is only implicitly denied if nobody else gives access explicitly).

<?php
    $permission
= $route->getRequirement('_permission');
   
// @todo Replace user_access() with a correctly injected and session-using
    //   alternative.
    // If user_access() fails, return NULL to give other checks a chance.
   
return user_access($permission) ? TRUE : NULL;
?>

Why would you ever want that?

Issue tags:+Stalking Crell

Let's see what Crell has to say about that when he's back from holiday.

I know this was modelled after hook_node_access() but that seems like a completely different situation to me.

For hook_node_access(), the caller doesn't specify what kind of checks he wants to have applied, he just asks, "Has anyone an opinion on whether this node should be accessible to the current user?". There it's obvious that an implementation should only explicitly grant or deny if it has something to say.

However, for a route, the "caller"/definition explicitly specifies what kind of checks he wants to have applied. He defines which implementations he wants to invoke with which configuration. Why would such a definition as above ever be an "grant access if either that or the other thing is true?" :)

Note: I'm not saying to change the access handling logic. I'm just suggesting that implementations like _permission or the entity access stuff should return either TRUE or FALSE and not NULL.

Ok, enough ranting :)

@Berdir makes total sense. I'd like to hears @Crell's point of view on this.

As far as I understand the access checker it's a system where the access check manager asks: Who can tell us something about the access to a route?
Can you 100% say the user should have access, or can you say the user should not have access, or you can't answer the question.

As far as I understand is that there is a need for OR based access. For example on a view, you want to have a permission called "access all views",
but also a specific access like per role. In general it seems to be that you should be able to specify whether you want to connect the parts with AND/OR. Maybe this would solve the confusion?

Currently OR brings a lot of troubles - once we trying to convert csrf-token protected pages we stuck with #1798296: Integrate CSRF link token directly into routing system
It means that no way to get success from csrfAccess AND check other permissions

@dawehner: That's a nice counter example but that use case only works because you have two different kind of checks (permission + role). You can't do that for the admin permission + configured permission anyway for example.

Also "Who can tell us something about the access to a route?" is IMHO not true. As said above, it's true for node/entity access, but for routes, we have defined the access checked that should be done explicitly, those classes are just the implementation of our definition.

Shouldn't the whole views access logic be encapsulated in a views entity access controller anyway?

Shouldn't the whole views access logic be encapsulated in a views entity access controller anyway?

Well, if you could just call $view->access() all the time, this statement would be certainly true. It's another case when you do have to make access
in a performant way. I really want to avoid the need to load the view object, all kind of plugins etc. to just determine the access. The route based access system seems to be a good way to handle this need.

@berdir
Just in case you are interested there is an issue to have an access controller: #1870758: Implement an entity access controller for a view

Status:Needs review» Needs work

correct status

StatusFileSize
new1.3 KB
new9.23 KB
FAILED: [[SimpleTest]]: [MySQL] 55,606 pass(es), 58 fail(s), and 23 exception(s).
[ View ]

I've moved 'access news feeds' back into the access controller so the are AND'ed and rerolled on latest head.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1974376-aggregator-categories-controller-22.patch, failed testing.

Just an FYI, I'm not ignoring this issue but the stalking bits seem related to: #1986640: Support AND/OR conjunctions for permission checks and #1984528: Follow-up: Allow for more robust access checking, so I've commented on those.

Issue tags:-Stalking Crell

Works for me, following those issues and releasing you from this one ;)

+++ b/core/modules/aggregator/aggregator.routing.ymlundefined
@@ -39,3 +39,10 @@ aggregator_opml_add:
+    _controller: '\Drupal\aggregator\Routing\AggregatorController::categories'

Shouldn't that be rather _content instead?

Status:Needs work» Needs review
StatusFileSize
new1.2 KB
new9.07 KB
PASSED: [[SimpleTest]]: [MySQL] 55,526 pass(es).
[ View ]

Changed to _content as per #27.

This also fixed an issue where the render array in adminOverview() wasn't being returned. I think this is what is making all the tests fail.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -153,4 +165,40 @@ public function adminOverview() {
+  public function categories() {
+

unneeded newline

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -153,4 +165,40 @@ public function adminOverview() {
+    module_load_include('inc', 'aggregator', 'aggregator.pages');

lets use \Drupal::moduleHandler

StatusFileSize
new1.02 KB
new9.09 KB
PASSED: [[SimpleTest]]: [MySQL] 55,596 pass(es).
[ View ]

Adds fixes in #29

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.php
@@ -153,4 +165,39 @@ public function adminOverview() {
+    // @todo Refactor this once all controller conversions are complete.
+    \Drupal::moduleHandler()->loadInclude('aggregator', 'inc', 'aggregator.pages');

Why not just go ahead and inject the module_handler service? It's unclear to me if we can get away with that post-July 1 at this point, so let's do it while we're here.

StatusFileSize
new2.65 KB
new9.49 KB
PASSED: [[SimpleTest]]: [MySQL] 55,622 pass(es).
[ View ]

Injecting all the things...

Status:Needs review» Reviewed & tested by the community

thanks kim.pepper!

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

No longer applies... needs a reroll...

curl https://drupal.org/files/1974376-aggregator-categories-controller-32.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  9715  100  9715    0     0  12264      0 --:--:-- --:--:-- --:--:-- 28406
error: patch failed: core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.php:7
error: core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.php: patch does not apply

Status:Needs work» Needs review
StatusFileSize
new9.84 KB
PASSED: [[SimpleTest]]: [MySQL] 56,009 pass(es).
[ View ]

Re-roll.

Status:Needs review» Reviewed & tested by the community

Setting back to RTBC (assuming green).

StatusFileSize
new7.97 KB
PASSED: [[SimpleTest]]: [MySQL] 55,763 pass(es).
[ View ]

Re-roll.

Status:Reviewed & tested by the community» Needs work
Issue tags:-Needs reroll, -WSCCI-conversion

The last submitted patch, 1974376-aggregator-categories-controller-36.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Needs reroll, +WSCCI-conversion

Status:Reviewed & tested by the community» Needs review
Issue tags:+Needs reroll
StatusFileSize
new7.82 KB
PASSED: [[SimpleTest]]: [MySQL] 55,616 pass(es).
[ View ]

Re-roll.

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs reroll

Looks good to me, thanks for keeping it going @kim.pepper

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs reroll
StatusFileSize
new7.53 KB
PASSED: [[SimpleTest]]: [MySQL] 55,052 pass(es).
[ View ]

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -212,6 +212,41 @@ public function adminOverview() {
+      $category->url = url('aggregator/categories/' . $category->cid);

we can replace url with the an injected url generator service.

Status:Needs work» Needs review
StatusFileSize
new4.72 KB
new11.38 KB
PASSED: [[SimpleTest]]: [MySQL] 57,659 pass(es).
[ View ]

Injected PathBasedGeneratorInterface for all url generation as per #43.

Status:Needs review» Needs work
Issue tags:-WSCCI-conversion

The last submitted patch, 1974376-aggregator-categories-controller-44.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+WSCCI-conversion

Status:Needs review» Reviewed & tested by the community

we might want to split the controller later, its __construct start getting big:)

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Needs a reroll as the AggregatorController has moved to core/modules/aggregator/lib/Drupal/aggregator/Controller :)

curl https://drupal.org/files/1974376-aggregator-categories-controller-44.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 11651  100 11651    0     0   4795      0  0:00:02  0:00:02 --:--:--  8158
error: patch failed: core/modules/aggregator/aggregator.routing.yml:60
error: core/modules/aggregator/aggregator.routing.yml: patch does not apply
error: core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.php: does not exist in index

Status:Needs work» Needs review
StatusFileSize
new11.5 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Rerolled.

Status:Needs review» Needs work
StatusFileSize
new2.54 KB
new11.4 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Rerolled against 8.x HEAD.

Status:Needs work» Needs review

Why do I always forget to change the status ... :|

Uh oh! haha, hopefully one of us gets the green light. :D

Ha. Good practice for me. :-)

IN BOTH:

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.phpundefined
@@ -52,6 +53,13 @@ class AggregatorController implements ControllerInterface {
+   *
+   * @var  \Drupal\Core\Routing\PathBasedGeneratorInterface

Extra space after @var

IN #50:

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.phpundefined
@@ -80,7 +89,8 @@ public static function create(ContainerInterface $container) {
++     $container->get('url_generator')

Extra + sign

IN #49:

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.phpundefined
@@ -52,7 +53,14 @@ class AggregatorController implements ControllerInterface {
-   * Constructs a \Drupal\aggregator\Controller\AggregatorController object.
...
+   * Constructs a \Drupal\aggregator\Routing\AggregatorController object.

This is in Controller, not Routing.

----
One of you can reroll one of the patches :)

StatusFileSize
new427 bytes
new11.38 KB
FAILED: [[SimpleTest]]: [MySQL] 57,786 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Werid about that extra plus sign - still ran fine for me.

Anyhoo, updated.

Issue tags:-Needs reroll

Removed tag.

Status:Needs review» Needs work

The last submitted patch, 1974376-aggregator-categories-controller-55.patch, failed testing.

Just a tiny nitpick.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.phpundefined
@@ -52,6 +53,13 @@ class AggregatorController implements ControllerInterface {
   protected $moduleHandler;
   /**

Needs one more empty line.

Gah, will address these errors later when I have more time.

Status:Needs work» Needs review
StatusFileSize
new6.71 KB
new9.78 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Patch reroll to fix the errors

As for the nitpick item above, I couldn't find it in the file, so may have been fixed already.

StatusFileSize
new1.01 KB
new11.4 KB
PASSED: [[SimpleTest]]: [MySQL] 58,127 pass(es).
[ View ]

Errr forgot to include a file, so reroll

Status:Needs review» Reviewed & tested by the community

thanks

Status:Reviewed & tested by the community» Fixed

Committed 08f15f2 and pushed to 8.x. Thanks!

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

Issue summary:View changes

Link to meta task