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

CommentFileSizeAuthor
#61 1974376-aggregator-categories-controller-61.patch11.4 KBstella
#61 1974376-55-61-interdiff.txt1.01 KBstella
#60 1974376-aggregator-categories-controller-60.patch9.78 KBstella
#60 1974376-55-60-interdiff.txt6.71 KBstella
#55 1974376-aggregator-categories-controller-55.patch11.38 KBCB
#55 1974376-49-55-interdiff.txt427 bytesCB
#50 1974376-aggregator-categories-controller-49.patch11.4 KBCB
#50 1974376-44-49-interdiff.txt2.54 KBCB
#49 1974376-aggregator-categories-controller-49.patch11.5 KBpwieck
#44 1974376-aggregator-categories-controller-44.patch11.38 KBkim.pepper
#44 interdiff.txt4.72 KBkim.pepper
#42 1974376-aggregator-categories-controller-42.patch7.53 KBkim.pepper
#40 1974376-aggregator-categories-controller-40.patch7.82 KBkim.pepper
#37 1974376-aggregator-categories-controller-36.patch7.97 KBkim.pepper
#35 1974376-aggregator-categories-controller-35.patch9.84 KBkim.pepper
#32 1974376-aggregator-categories-controller-32.patch9.49 KBkim.pepper
#32 interdiff.txt2.65 KBkim.pepper
#30 1974376-aggregator-categories-controller-30.patch9.09 KBkim.pepper
#30 interdiff.txt1.02 KBkim.pepper
#28 1974376-aggregator-categories-controller-28.patch9.07 KBkim.pepper
#28 interdiff.txt1.2 KBkim.pepper
#22 1974376-aggregator-categories-controller-22.patch9.23 KBkim.pepper
#22 interdiff.txt1.3 KBkim.pepper
#11 1974376-aggregator-categories-controller-11.patch9.15 KBkim.pepper
#11 interdiff.txt1.3 KBkim.pepper
#10 1974376-aggregator-categories-controller-10.patch9.15 KBkim.pepper
#10 interdiff.txt1.77 KBkim.pepper
#7 1974376-aggregator-categories-controller-7.patch9.07 KBkim.pepper
#7 interdiff.txt639 byteskim.pepper
#5 1974376-aggregator-categories-controller-5.patch9.08 KBkim.pepper
#5 interdiff.txt3.33 KBkim.pepper
#4 1974376-aggregator-categories-controller-4.patch6.44 KBkim.pepper
#4 interdiff.txt3.57 KBkim.pepper
#1 1974376-aggregator-categories-controller-1.patch4.63 KBkim.pepper
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kim.pepper’s picture

Status: Active » Needs review
FileSize
4.63 KB

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.

kim.pepper’s picture

Assigned: Unassigned » kim.pepper
ParisLiakos’s picture

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

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
3.57 KB
6.44 KB

Injecting all the dependencies.

Still to create an AccessCheck

kim.pepper’s picture

Now with accesscheck implementation.

dawehner’s picture

+++ 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.

kim.pepper’s picture

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

Updated comment.

Berdir’s picture

+++ 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...

dawehner’s picture

+++ 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

kim.pepper’s picture

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

kim.pepper’s picture

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

Berdir’s picture

+++ 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).

    $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?

Berdir’s picture

Issue tags: +Stalking Crell

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

Berdir’s picture

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 :)

kim.pepper’s picture

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

dawehner’s picture

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?

andypost’s picture

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

Berdir’s picture

@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?

dawehner’s picture

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.

dawehner’s picture

@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

ParisLiakos’s picture

Status: Needs review » Needs work

correct status

kim.pepper’s picture

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

kim.pepper’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

Crell’s picture

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.

Berdir’s picture

Issue tags: -Stalking Crell

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

dawehner’s picture

+++ 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?

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
1.2 KB
9.07 KB

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.

ParisLiakos’s picture

+++ 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

kim.pepper’s picture

Crell’s picture

+++ 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.

kim.pepper’s picture

Injecting all the things...

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

thanks kim.pepper!

alexpott’s picture

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
kim.pepper’s picture

Status: Needs work » Needs review
FileSize
9.84 KB

Re-roll.

kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community

Setting back to RTBC (assuming green).

kim.pepper’s picture

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.

kim.pepper’s picture

Status: Needs work » Needs review
Issue tags: +Needs reroll, +WSCCI-conversion
kim.pepper’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs reroll
FileSize
7.82 KB

Re-roll.

tim.plunkett’s picture

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

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

kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
7.53 KB
alexpott’s picture

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.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
4.72 KB
11.38 KB

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.

ParisLiakos’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI-conversion
ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

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
pwieck’s picture

Status: Needs work » Needs review
FileSize
11.5 KB

Rerolled.

CB’s picture

Status: Needs review » Needs work
FileSize
2.54 KB
11.4 KB

Rerolled against 8.x HEAD.

CB’s picture

Status: Needs work » Needs review

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

CB’s picture

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

pwieck’s picture

Ha. Good practice for me. :-)

tim.plunkett’s picture

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 :)

CB’s picture

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

Anyhoo, updated.

Gaelan’s picture

Issue tags: -Needs reroll

Removed tag.

Status: Needs review » Needs work

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

dawehner’s picture

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.

CB’s picture

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

stella’s picture

Status: Needs work » Needs review
FileSize
6.71 KB
9.78 KB

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.

stella’s picture

Errr forgot to include a file, so reroll

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

thanks

alexpott’s picture

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.

Anonymous’s picture

Issue summary: View changes

Link to meta task