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

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

CommentFileSizeAuthor
#76 layout-1978910-76.patch6.71 KBtim.plunkett
#76 interdiff.txt2.15 KBtim.plunkett
#73 layout-1978910-73.patch7.13 KBkgoel
#73 interdiff.txt2.03 KBkgoel
#60 layout-1978910-60.patch6.15 KBtim.plunkett
#60 interdiff.txt1.08 KBtim.plunkett
#58 layout-1978910-58-FAIL.patch856 bytestim.plunkett
#58 layout-1978910-58-PASS.patch5.99 KBtim.plunkett
#51 1978910-controller-51.patch6.47 KBkgoel
#51 interdiff.txt1.22 KBkgoel
#43 1978910-controller-43.patch6.47 KBkgoel
#43 interdiff.txt6.61 KBkgoel
#39 drupal-1978910-39.patch6.47 KBdawehner
#39 interdiff.txt3.8 KBdawehner
#38 1978910-controller-38.patch5.79 KBkgoel
#38 interdiff.txt655 byteskgoel
#36 1978910-controller-36.patch5.86 KBkgoel
#36 interdiff.txt575 byteskgoel
#35 1978910-controller-35.patch5.86 KBkgoel
#35 interdiff.txt615 byteskgoel
#33 1978910-controller-33.patch6.97 KBvijaycs85
#33 1978910-diff-23-33.txt2.79 KBvijaycs85
#23 1978910-controller-23.patch5.76 KBkgoel
#23 interdiff.txt1.14 KBkgoel
#18 1978910-controller-18.patch4.78 KBkgoel
#18 interdiff.txt706 byteskgoel
#15 1978910-controller-15.patch4.74 KBkgoel
#15 interdiff.txt709 byteskgoel
#13 1978910-controller-13.patch4.8 KBkgoel
#13 interdiff.txt793 byteskgoel
#12 1978910-controller-12.patch4.81 KBkgoel
#12 interdiff.txt2.19 KBkgoel
#10 1978910-controller-10.patch5.52 KBkgoel
#8 1978910-controller-8.patch3.62 KBkgoel
#8 interdiff.txt1.42 KBkgoel
#6 1978910-controller-6.patch3.62 KBkgoel
#4 1978910-controller-4.patch3.84 KBkgoel
#4 interdiff.txt659 byteskgoel
#2 1978910-controller-2.patch3.85 KBkgoel
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kgoel’s picture

Assigned: Unassigned » kgoel

going to work on this issue.

kgoel’s picture

Status: Active » Needs review
FileSize
3.85 KB

Patch attached.

Status: Needs review » Needs work

The last submitted patch, 1978910-controller-2.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review
FileSize
659 bytes
3.84 KB

Trying again.

Status: Needs review » Needs work

The last submitted patch, 1978910-controller-4.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review
FileSize
3.62 KB

Created new patch since drupal.org/node/1978908 was committed.

Status: Needs review » Needs work

The last submitted patch, 1978910-controller-6.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review
FileSize
1.42 KB
3.62 KB

Trying again.

kim.pepper’s picture

Looking good, however, the access check callback is not being called anymore.

+++ b/core/modules/layout/layout.routing.ymlundefined
@@ -4,3 +4,9 @@ layout_page_list:
+    layout_user_access: 'TRUE'

Need to create an implementation of AccessCheckInterface.

kgoel’s picture

FileSize
5.52 KB

Thanks Kim!

I have added AccessCheckInterface function.

dawehner’s picture

+++ b/core/modules/layout/layout.routing.ymlundefined
@@ -4,3 +4,9 @@ layout_page_list:
+  pattern: '/admin/structure/templates/manage/{node}'

{node} probably does not work, and it also feels wrong at that point.

+++ b/core/modules/layout/lib/Drupal/layout/Controller/LayoutController.phpundefined
@@ -76,4 +76,22 @@ public function layoutPageList() {
+  public function layoutPageView($key) {

Needs docs

+++ b/core/modules/layout/lib/Drupal/layout/Controller/LayoutController.phpundefined
@@ -76,4 +76,22 @@ public function layoutPageList() {
+    $layout = layout_manager()->getDefinition($key);
...
+    $instance = layout_manager()->createInstance($key, array());

let's inject the layout manager into the layout controller...

+++ b/core/modules/layout/lib/Drupal/layout/Controller/LayoutController.phpundefined
@@ -76,4 +76,22 @@ public function layoutPageList() {
+  }

Nitpick: empty line missing.

kgoel’s picture

Addressed all the issues under comment# 11.

kgoel’s picture

FileSize
793 bytes
4.8 KB

Fixed some white spaces in layout controller.

dawehner’s picture

Here are just some nitpicks.

+++ b/core/modules/layout/lib/Drupal/layout/Controller/LayoutController.phpundefined
@@ -76,4 +76,34 @@ public function layoutPageList() {
+   * Page callback: Demonstrates a layout template.

Let's remove the "Page callback"

+++ b/core/modules/layout/lib/Drupal/layout/Controller/LayoutController.phpundefined
@@ -76,4 +76,34 @@ public function layoutPageList() {
+   * ¶
+   * @param string $key
+   *   The key of the page layout being requested.
+   *   ¶

Here is some trailing whitespace.

kgoel’s picture

FileSize
709 bytes
4.74 KB

Removed page callback and white spaces.

Status: Needs review » Needs work

The last submitted patch, 1978910-controller-15.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
+++ b/core/modules/layout/lib/Drupal/layout/Controller/LayoutController.phpundefined
@@ -76,4 +76,32 @@ public function layoutPageList() {
+  /**
...
+  public function layoutPageView($key) {

Ups, this method should still have a documentation. sorry

kgoel’s picture

FileSize
706 bytes
4.78 KB

Added doc block.

Status: Needs review » Needs work

The last submitted patch, 1978910-controller-18.patch, failed testing.

dawehner’s picture

You forgot to add the LayoutAccessCheck class

kgoel’s picture

You forgot to add the LayoutAccessCheck class

I have added LayoutAccessCheck in layout.services.yml. Not sure where else I need to add that class.

dawehner’s picture

You need to write also the LayoutAccessCheck class, dealing with that.

kgoel’s picture

Status: Needs work » Needs review
FileSize
1.14 KB
5.76 KB

Trying again with LayoutAccessCheck.php

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

The last submitted patch, 1978910-controller-23.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review

#23: 1978910-controller-23.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1978910-controller-23.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review

#23: 1978910-controller-23.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1978910-controller-23.patch, failed testing.

dawehner’s picture

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

#23: 1978910-controller-23.patch queued for re-testing.

alexpott’s picture

xmacinfo’s picture

Priority: Normal » Major

Moving to major since some essential pages are blocked, like user profile.

Is there an issue to convert the same type of issue for the tracker module?

dawehner’s picture

Status: Needs review » Needs work
+++ b/core/modules/layout/lib/Drupal/layout/Access/LayoutAccessCheck.phpundefined
@@ -0,0 +1,32 @@
+    return user_access('administer layouts') && layout_manager()->getDefinition($key);

Let's also inject the layout manager into the access class.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
2.79 KB
6.97 KB

@dawehner, not sure how to inject it on access class (tried the same way we have in controller, but no luck). Fixed few request related issues and looks the page is working fine now.

Crell’s picture

Status: Needs review » Needs work
+++ b/core/modules/layout/layout.services.yml
@@ -2,3 +2,8 @@ services:
+  access_check.layout:
+    class: Drupal\layout\Access\LayoutAccessCheck
+    tags:
+      - { name: access_check }
+

Add an arguments block here to specify the service to inject into the access checker's constructor. See:

http://drupal.org/node/1953354

For an example. (You'd use a different service name, of course, but same idea.)

+++ b/core/modules/layout/lib/Drupal/layout/Controller/LayoutController.php
@@ -20,7 +21,7 @@ class LayoutController implements ControllerInterface {
-  protected $layoutManager;
+  protected $layout_manager;

This change is wrong. Object properties should always be lowerCamelCase.

kgoel’s picture

Status: Needs work » Needs review
FileSize
615 bytes
5.86 KB

Added argument in the services.yml

kgoel’s picture

FileSize
575 bytes
5.86 KB

fixed white space

dawehner’s picture

Status: Needs review » Needs work
+++ b/core/modules/layout/layout.services.ymlundefined
@@ -2,3 +2,10 @@ services:
+    arguments: ['@layout_manager']   ¶

Please remove the extra whitespaces (http://vim.wikia.com/wiki/Highlight_unwanted_spaces would have helped you here :) ) If you copy this line to the access_check.layout your have the layout manager injected.

kgoel’s picture

Status: Needs work » Needs review
FileSize
655 bytes
5.79 KB
dawehner’s picture

FileSize
3.8 KB
6.47 KB

Let me help you.

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

The last submitted patch, drupal-1978910-39.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review

#39: drupal-1978910-39.patch queued for re-testing.

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

The last submitted patch, drupal-1978910-39.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review
FileSize
6.61 KB
6.47 KB

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

The last submitted patch, 1978910-controller-43.patch, failed testing.

kim.pepper’s picture

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

#43: 1978910-controller-43.patch queued for re-testing.

dawehner’s picture

That's RTBC once the testbot gives an OK.

clemens.tolboom’s picture

This patch somehow fixes 'access denied' on user/1/view + user/1/edit .. how come?

xmacinfo’s picture

@clemens.tolboom: Please look at #30.

clemens.tolboom’s picture

Status: Needs review » Reviewed & tested by the community

@xmacinfo: tnx (and doh :-)

I can access admin/structure/templates after applying the patch. It needs Cache Clear twice but that's another issue I guess.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/layout/layout.routing.ymlundefined
@@ -4,3 +4,9 @@ layout_page_list:
+    layout_user_access: 'TRUE'

+++ b/core/modules/layout/lib/Drupal/layout/Access/LayoutAccessCheck.phpundefined
@@ -0,0 +1,44 @@
+    return array_key_exists('layout_user_access', $route->getRequirements());

We prefix these with an underscore to prevent clashes with {placeholders}

kgoel’s picture

Status: Needs work » Needs review
FileSize
1.22 KB
6.47 KB
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Good point tim!

tim.plunkett’s picture

Priority: Major » Normal

This is no more major than other conversions

xmacinfo’s picture

Priority: Normal » Major

Other conversions are working before the conversion. Whereas here, Drupal is broken without the fix, which fix happens to be a conversion.

tim.plunkett’s picture

Category: task » bug
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

If this is a bug, it needs tests. I strongly disagree with the priority.

kgoel’s picture

xmacinfo - I am not sure if this patch is breaking something or this patch is fixing something which is already broken. Can you please elaborate little more?

xmacinfo’s picture

@kgoel: “this patch is fixing something which is already broken”. See Comment #30.

In short, when we enable Layout, we get an access denied on any profile pages, even if we are user 1.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
5.99 KB
856 bytes

Thanks for describing the bug. Here's a regression test.

dawehner’s picture

Status: Needs review » Needs work
+++ b/core/modules/layout/lib/Drupal/layout/Access/LayoutAccessCheck.phpundefined
@@ -0,0 +1,44 @@
+  public function __construct(PluginManagerInterface $layout_manager) {
+    $this->layoutManager = $layout_manager;
+  }

This variable should be documented.

+++ b/core/modules/layout/lib/Drupal/layout/Access/LayoutAccessCheck.phpundefined
@@ -0,0 +1,44 @@
+    return user_access('administer layouts') && $this->layoutManager->getDefinition($request->attributes->get('key'));

access checkers should return static::ALLOW and static::DENY

+++ b/core/modules/layout/lib/Drupal/layout/Tests/LayoutDerivativesTest.phpundefined
@@ -107,4 +107,14 @@ function testPageLayout() {
+
+  /**
+   * Ensure layout does not restrict access to user pages.
+   */
+  public function testUserAccessRegression() {
+    $this->drupalLogin($this->root_user);
+    $this->assertUrl('user/' . $this->root_user->uid);
+    $this->assertResponse(200);
+  }

Can you explain how this tests the patch?

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.08 KB
6.15 KB

I didn't spend the time to figure out WHY the layout module returns 403 for all user pages, including user 1, but it is what happens, and the patch fixes the cause.
That's why I named it "testUserAccessRegression".
It probably needs more research and a real test.

Fixed the first two points.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

The problem before has been the following: UserAccessController executes the hook_user_access, but layout_user_access was named like that.

Dries’s picture

Given that we're pretty much at the end of the development cycle and layout module isn't actually used it core, it may be better to remove layout module from core? Thoughts?

Crell’s picture

Assigned: kgoel » EclipseGc
Issue tags: +scotch

That's a SCOTCH question. I don't know if that would make life easier or harder for them. I defer to EclipseGc and Sam. Pinging them.

YesCT’s picture

Issue tags: +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

Dries’s picture

Waiting to hear from EclipseGc before moving forward.

EclipseGc’s picture

From a code perspective, this seems fine, so I'm ++ there.

With regard to whether we should remove this, I would encourage us to tag it and come back before release. Nothing in core is using it yet, that's true, but Sam and I have made great strides towards using it, and looking at Dries' blog post, it seems we still could put in code that will use it so long as we're not changing APIs. I agree this should absolutely be removed before 8.0 (or sooner) if we don't have code that is using it, but it's also rather small, and self contained, so if we need to remove it later, it should be of minimal impact, and its presence allows sam and I to continue making progress without maintaining yet more code differences in princess.

If this is contentious, we can talk it out further, but the code itself here seems pretty straight forward, so I'd say we commit it.

Eclipse

xmacinfo’s picture

@EclipseGc: Which tag should we use or create?

EclipseGc’s picture

Do we have a "review before release" or something?

Eclipse

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Looks like we should be removing layout_page_view() from layout.admin.inc in this patch...

+++ b/core/modules/layout/lib/Drupal/layout/Tests/LayoutDerivativesTest.phpundefined
@@ -107,4 +107,14 @@ function testPageLayout() {
+  /**
+   * Ensure layout does not restrict access to user pages.
+   */
+  public function testUserAccessRegression() {
+    $this->drupalLogin($this->root_user);
+    $this->assertUrl('user/' . $this->root_user->uid);
+    $this->assertResponse(200);
+  }

Given that we have to reinstall drupal just to run this test and give that you are removing the offending hook... I think that this test is unnecessary. Funny bug though :)

star-szr’s picture

Assigned: EclipseGc » Unassigned

We have a revisit before release tag.

alexpott’s picture

Assigned: Unassigned » EclipseGc

In fact we should be removing the whole of layout.admin.inc

alexpott’s picture

Assigned: EclipseGc » Unassigned

x-post

kgoel’s picture

Status: Needs work » Needs review
FileSize
2.03 KB
7.13 KB
dawehner’s picture

+++ b/core/modules/layout/lib/Drupal/layout/Access/LayoutAccessCheck.phpundefined
@@ -0,0 +1,51 @@
+    return user_access('administer layouts') && $this->layoutManager->getDefinition($request->attributes->get('key')) ? static::ALLOW : static::DENY;

I would vote to go with _permission: administer layouts and the custom layout access checker here, so there is just a single one.

On the route definition you have to set a new option: _access_mode: 'ALL' in order to work as you expect it to work.

dawehner’s picture

Status: Needs review » Needs work
tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
2.15 KB
6.71 KB
jibran’s picture

+++ b/core/modules/layout/lib/Drupal/layout/Controller/LayoutController.phpundefined
@@ -76,4 +78,31 @@ public function layoutPageList() {
+      $regions[$region] = '<div class="layout-region-demonstration">' . String::checkPlain($info['label']) . '</div>';

Can we use https://drupal.org/node/2019739 here.

tim.plunkett’s picture

No. If we wanted to use something, we'd use #type container, but it doesn't expect a render array here.

safetypin’s picture

I just arrived here from #1986140. I (just this morning) downloaded d8, created a new database, performed a minimal installation, enabled the layout module and ran into the access denied error mentioned in that thread. I just downloaded this patch file, applied it, and seems to have resolved that problem for me.

I'm not sure what else needs to be tested..

xmacinfo’s picture

Issue tags: +revisit before beta

@Dries and @alexpott are both suggesting to remove the layout module from core, whereas @EclipseGC would like the patch to be commited and decide near release if we keep layout.

Adding the “revisit before release” tag as requested.

effulgentsia’s picture

Issue tags: -revisit before beta

Opened #2053879: Remove layout.module from core, so removing the "revisit before release" tag from this issue.

ergophobe’s picture

Just wanted to say that I updated from git on 11 Aug 2013 and applied the patch in #76 and it fixes all issues I was having #2062811: Access Denied to User 1 to View User Pages, Operations Column empty
- access denied on user pages
- consequent missing buttons on People page
- illegal offset warnings #1986140: User 1 profile Access Denied (view and edit) -> Warning Illegal offset type in isset or empty

Crell’s picture

Status: Needs review » Needs work

It sounds from #79 like this patch still has issues, although it looks good to me visually.

Whether layout stays in core or moves to contrib, this code will need to get written anyway so we may as well do it here while the module still lives here.

ergophobe’s picture

Crell - I think you mean the patch in #76.

#79 is a test reporting the issue encountered on a minimal install and fixed by the patch in #76.
#82 is reporting that the patch fixed all related issues for me based on a 8.x git pull of 11 Aug.

That's two positive tests, plus I assume tim.plunket wouldn't have posted the patch if it wasn't working for him. So with three people reporting the issue fixed and nobody contrary, it seems like it would be worth committing and closing this issue.

It seems like the issues are "philosophical" rather than practical (in other words, while deciding whether this should be in core or not, committing it will make it simpler for people to test other issues and patches - it probably took me at least an hour of looking at error messages, digging into code, posting issues, digging further and finding this patch here and then my time was up and I don't even remember what issue I was working on at the time).

Crell’s picture

Status: Needs work » Reviewed & tested by the community

Ah, OK. I misunderstood the context of #79. Let's commit it then.

catch’s picture

Assigned: Unassigned » Dries

If we're going to remove layout, I'd rather just remove it asap than be working on issues in the core queue when there's about 90 RTBC patches per week. Moving over to Dries.

xmacinfo’s picture

Crell’s picture

Fine, but this is one of those RTBCs. Can we get this committed before more work needs to be done on it, wherever it would be done?

webchick’s picture

#76: layout-1978910-76.patch queued for re-testing.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Since this'll need to happen regardless if Layout module gets removed from core, I figured it best to just go ahead and commit it.

Committed and pushed to 8.x. Thanks!

webchick’s picture

Assigned: Dries » Unassigned

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