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.

Files: 
CommentFileSizeAuthor
#76 layout-1978910-76.patch6.71 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,335 pass(es).
[ View ]
#76 interdiff.txt2.15 KBtim.plunkett
#73 layout-1978910-73.patch7.13 KBkgoel
PASSED: [[SimpleTest]]: [MySQL] 56,814 pass(es).
[ View ]
#73 interdiff.txt2.03 KBkgoel
#60 layout-1978910-60.patch6.15 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,907 pass(es).
[ View ]
#60 interdiff.txt1.08 KBtim.plunkett
#58 layout-1978910-58-FAIL.patch856 bytestim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,624 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#58 layout-1978910-58-PASS.patch5.99 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,322 pass(es).
[ View ]
#51 1978910-controller-51.patch6.47 KBkgoel
PASSED: [[SimpleTest]]: [MySQL] 55,996 pass(es).
[ View ]
#51 interdiff.txt1.22 KBkgoel
#43 1978910-controller-43.patch6.47 KBkgoel
PASSED: [[SimpleTest]]: [MySQL] 55,903 pass(es).
[ View ]
#43 interdiff.txt6.61 KBkgoel
#39 drupal-1978910-39.patch6.47 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1978910-39.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#39 interdiff.txt3.8 KBdawehner
#38 1978910-controller-38.patch5.79 KBkgoel
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#38 interdiff.txt655 byteskgoel
#36 1978910-controller-36.patch5.86 KBkgoel
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]
#36 interdiff.txt575 byteskgoel
#35 1978910-controller-35.patch5.86 KBkgoel
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]
#35 interdiff.txt615 byteskgoel
#33 1978910-controller-33.patch6.97 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 57,275 pass(es).
[ View ]
#33 1978910-diff-23-33.txt2.79 KBvijaycs85
#23 1978910-controller-23.patch5.76 KBkgoel
PASSED: [[SimpleTest]]: [MySQL] 55,810 pass(es).
[ View ]
#23 interdiff.txt1.14 KBkgoel
#18 1978910-controller-18.patch4.78 KBkgoel
FAILED: [[SimpleTest]]: [MySQL] 56,681 pass(es), 4 fail(s), and 2 exception(s).
[ View ]
#18 interdiff.txt706 byteskgoel
#15 1978910-controller-15.patch4.74 KBkgoel
FAILED: [[SimpleTest]]: [MySQL] 55,871 pass(es), 4 fail(s), and 2 exception(s).
[ View ]
#15 interdiff.txt709 byteskgoel
#13 1978910-controller-13.patch4.8 KBkgoel
FAILED: [[SimpleTest]]: [MySQL] 55,826 pass(es), 4 fail(s), and 2 exception(s).
[ View ]
#13 interdiff.txt793 byteskgoel
#12 1978910-controller-12.patch4.81 KBkgoel
FAILED: [[SimpleTest]]: [MySQL] 56,913 pass(es), 4 fail(s), and 2 exception(s).
[ View ]
#12 interdiff.txt2.19 KBkgoel
#10 1978910-controller-10.patch5.52 KBkgoel
PASSED: [[SimpleTest]]: [MySQL] 55,799 pass(es).
[ View ]
#8 1978910-controller-8.patch3.62 KBkgoel
PASSED: [[SimpleTest]]: [MySQL] 55,568 pass(es).
[ View ]
#8 interdiff.txt1.42 KBkgoel
#6 1978910-controller-6.patch3.62 KBkgoel
FAILED: [[SimpleTest]]: [MySQL] 55,600 pass(es), 4 fail(s), and 2 exception(s).
[ View ]
#4 1978910-controller-4.patch3.84 KBkgoel
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/layout/lib/Drupal/layout/Controller/LayoutController.php.
[ View ]
#4 interdiff.txt659 byteskgoel
#2 1978910-controller-2.patch3.85 KBkgoel
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/layout/lib/Drupal/layout/Controller/LayoutController.php.
[ View ]

Comments

Assigned:Unassigned» kgoel

going to work on this issue.

Status:Active» Needs review
StatusFileSize
new3.85 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/layout/lib/Drupal/layout/Controller/LayoutController.php.
[ View ]

Patch attached.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new659 bytes
new3.84 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/layout/lib/Drupal/layout/Controller/LayoutController.php.
[ View ]

Trying again.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new3.62 KB
FAILED: [[SimpleTest]]: [MySQL] 55,600 pass(es), 4 fail(s), and 2 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new1.42 KB
new3.62 KB
PASSED: [[SimpleTest]]: [MySQL] 55,568 pass(es).
[ View ]

Trying again.

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.

StatusFileSize
new5.52 KB
PASSED: [[SimpleTest]]: [MySQL] 55,799 pass(es).
[ View ]

Thanks Kim!

I have added AccessCheckInterface function.

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

StatusFileSize
new2.19 KB
new4.81 KB
FAILED: [[SimpleTest]]: [MySQL] 56,913 pass(es), 4 fail(s), and 2 exception(s).
[ View ]

Addressed all the issues under comment# 11.

StatusFileSize
new793 bytes
new4.8 KB
FAILED: [[SimpleTest]]: [MySQL] 55,826 pass(es), 4 fail(s), and 2 exception(s).
[ View ]

Fixed some white spaces in layout controller.

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.

StatusFileSize
new709 bytes
new4.74 KB
FAILED: [[SimpleTest]]: [MySQL] 55,871 pass(es), 4 fail(s), and 2 exception(s).
[ View ]

Removed page callback and white spaces.

Status:Needs review» Needs work

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

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

StatusFileSize
new706 bytes
new4.78 KB
FAILED: [[SimpleTest]]: [MySQL] 56,681 pass(es), 4 fail(s), and 2 exception(s).
[ View ]

Added doc block.

Status:Needs review» Needs work

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

You forgot to add the LayoutAccessCheck class

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.

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

Status:Needs work» Needs review
StatusFileSize
new1.14 KB
new5.76 KB
PASSED: [[SimpleTest]]: [MySQL] 55,810 pass(es).
[ View ]

Trying again with LayoutAccessCheck.php

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

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

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.

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.

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

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

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?

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.

Status:Needs work» Needs review
StatusFileSize
new2.79 KB
new6.97 KB
PASSED: [[SimpleTest]]: [MySQL] 57,275 pass(es).
[ View ]

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

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.

Status:Needs work» Needs review
StatusFileSize
new615 bytes
new5.86 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]

Added argument in the services.yml

StatusFileSize
new575 bytes
new5.86 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]

fixed white space

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.

Status:Needs work» Needs review
StatusFileSize
new655 bytes
new5.79 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

StatusFileSize
new3.8 KB
new6.47 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1978910-39.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Let me help you.

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

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

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.

Status:Needs work» Needs review
StatusFileSize
new6.61 KB
new6.47 KB
PASSED: [[SimpleTest]]: [MySQL] 55,903 pass(es).
[ View ]

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

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

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

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

That's RTBC once the testbot gives an OK.

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

@clemens.tolboom: Please look at #30.

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.

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}

Status:Needs work» Needs review
StatusFileSize
new1.22 KB
new6.47 KB
PASSED: [[SimpleTest]]: [MySQL] 55,996 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

Good point tim!

Priority:Major» Normal

This is no more major than other conversions

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.

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.

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?

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

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new5.99 KB
PASSED: [[SimpleTest]]: [MySQL] 58,322 pass(es).
[ View ]
new856 bytes
FAILED: [[SimpleTest]]: [MySQL] 56,624 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

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

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?

Status:Needs work» Needs review
StatusFileSize
new1.08 KB
new6.15 KB
PASSED: [[SimpleTest]]: [MySQL] 57,907 pass(es).
[ View ]

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.

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.

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?

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.

Issue tags:+RTBC July 1

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

Waiting to hear from EclipseGc before moving forward.

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

@EclipseGc: Which tag should we use or create?

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

Eclipse

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

Assigned:EclipseGc» Unassigned

We have a revisit before release tag.

Assigned:Unassigned» EclipseGc

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

Assigned:EclipseGc» Unassigned

x-post

Status:Needs work» Needs review
StatusFileSize
new2.03 KB
new7.13 KB
PASSED: [[SimpleTest]]: [MySQL] 56,814 pass(es).
[ View ]

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

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new2.15 KB
new6.71 KB
PASSED: [[SimpleTest]]: [MySQL] 58,335 pass(es).
[ View ]

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

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

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

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.

Issue tags:-revisit before beta

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

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

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.

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

Status:Needs work» Reviewed & tested by the community

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

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.

Agreed! And this affects #2053879: Remove layout.module from core.

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?

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

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!

Assigned:Dries» Unassigned

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