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

Will cover:

  • menu_overview_page
  • menu_parent_options_js
  • menu_menu_add
  • menu_menu_edit
  • menu_link_add
  • entity_get_form (used for admin/structure/menu/item/%menu_link/edit)
CommentFileSizeAuthor
#70 menu-1984702-70.patch38.3 KBtim.plunkett
#68 menu-1984702-68-same.patch38.63 KBtim.plunkett
#68 menu-1984702-68-no-route.patch38.3 KBtim.plunkett
#68 interdiff.txt514 bytestim.plunkett
#66 menu-1984702-66.patch38.63 KBtim.plunkett
#66 interdiff.txt5.28 KBtim.plunkett
#64 menu-1984702-64.patch37.45 KBtim.plunkett
#59 menu-1984702-59.patch45.96 KBtim.plunkett
#59 interdiff.txt10.01 KBtim.plunkett
#57 menu-1984702-57.patch38.05 KBtim.plunkett
#56 menu-1984702-56.patch37.43 KBtim.plunkett
#56 interdiff.txt611 bytestim.plunkett
#54 menu-1984702-53.patch37.42 KBtim.plunkett
#51 menu-1984702-51.patch37.69 KBtim.plunkett
#48 menu-1984702-48.patch37.79 KBtim.plunkett
#48 interdiff.txt2.31 KBtim.plunkett
#44 menu-1984702-44.patch37.57 KBtim.plunkett
#41 menu-1984702-41-combined.patch44.02 KBtim.plunkett
#41 menu-1984702-41-do-not-test.patch37.58 KBtim.plunkett
#37 menu-1984702-37.patch36.68 KBtim.plunkett
#37 interdiff.txt3.29 KBtim.plunkett
#34 menu-1984702-34.patch36.1 KBtim.plunkett
#34 interdiff.txt932 bytestim.plunkett
#32 menu-1984702-32.patch36.09 KBtim.plunkett
#30 menu-1984702-30.patch36.05 KBtim.plunkett
#30 interdiff.txt22.37 KBtim.plunkett
#28 menu-1984702-28.patch18.97 KBtim.plunkett
#26 menu-1984702-26.patch19.61 KBtim.plunkett
#24 menu-1984702-24.patch19.6 KBtim.plunkett
#24 interdiff.txt5.26 KBtim.plunkett
#21 menu-1984702-21.patch19.51 KBdawehner
#21 interdiff.txt1.22 KBdawehner
#19 interdiff.txt2.02 KBdawehner
#19 menu-1984702-19.patch19.41 KBdawehner
#18 menu-1984702-18.patch18.47 KBtim.plunkett
#16 menu-1984702-16.patch19.94 KBtim.plunkett
#12 menu-1984702-12.patch20.04 KBtim.plunkett
#12 interdiff.txt3.08 KBtim.plunkett
#11 menu-1984702-11.patch21.16 KBtim.plunkett
#11 interdiff.txt15.17 KBtim.plunkett
#8 drupal-1984702-8.patch24.11 KBdawehner
#8 interdiff.txt10.54 KBdawehner
#6 drupal-1984702-6.patch15.83 KBdawehner
#1 menu-1984702-1.patch11.6 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
11.6 KB

Yay.

Status: Needs review » Needs work

The last submitted patch, menu-1984702-1.patch, failed testing.

dawehner’s picture

+++ b/core/modules/menu/lib/Drupal/menu/Controller/MenuController.phpundefined
@@ -0,0 +1,93 @@
+    if (isset($_POST['menus']) && count($_POST['menus'])) {
+      foreach ($_POST['menus'] as $menu) {

Can't we get just the request object injected from the controller?

+++ b/core/modules/menu/menu.routing.ymlundefined
@@ -5,6 +5,34 @@ menu_settings:
+    _access: 'TRUE'

Do we actually really want to do that? This feels like a potential security issue.

tim.plunkett’s picture

dawehner’s picture

Assigned: tim.plunkett » dawehner

Trying to fix the failures.

dawehner’s picture

Assigned: dawehner » Unassigned
Status: Needs work » Needs review
FileSize
15.83 KB
$perms = array_keys(module_invoke_all('permission'));
$this->admin_user = $this->drupalCreateUser($perms);

Drupal has some interesting bits of code :)

It seems to make sense to move this menu_edit_menu_name_exists() as it's just use in a single place (the controller).
Sadly the breadbrumb test still fail, but I couldn't figure out yet why they actually fail.

Status: Needs review » Needs work

The last submitted patch, drupal-1984702-6.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
10.54 KB
24.11 KB

This patch basically adds '/edit' to all paths, as routing doesn't work without it.

Does someone know of a central issue where this is discussed?

Status: Needs review » Needs work

The last submitted patch, drupal-1984702-8.patch, failed testing.

tim.plunkett’s picture

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
15.17 KB
21.16 KB

I hate breadcrumbs, breadcrumbs suck.

tim.plunkett’s picture

FileSize
3.08 KB
20.04 KB

Contextual links assume you want to go to MENU_DEFAULT_LOCAL_TASK. This will be a problem.

Status: Needs review » Needs work
Issue tags: -FormInterface, -WSCCI-conversion, -MENU_LOCAL_ACTION

The last submitted patch, menu-1984702-12.patch, failed testing.

The last submitted patch, menu-1984702-12.patch, failed testing.

tim.plunkett’s picture

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
19.94 KB

This incorporates that patch. Should pass menu tests just fine, but will likely break dozens of other tests.

Status: Needs review » Needs work

The last submitted patch, menu-1984702-16.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
18.47 KB

That went in!

dawehner’s picture

FileSize
19.41 KB
2.02 KB

At least the factory should be injected. Beside from that it seems perfect so far.

Status: Needs review » Needs work

The last submitted patch, menu-1984702-19.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.22 KB
19.51 KB

Ups.

Status: Needs review » Needs work
Issue tags: -FormInterface, -WSCCI-conversion, -MENU_LOCAL_ACTION

The last submitted patch, menu-1984702-21.patch, failed testing.

dawehner’s picture

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

#21: menu-1984702-21.patch queued for re-testing.

tim.plunkett’s picture

FileSize
5.26 KB
19.6 KB

Modernized the patch.

This now depends on #2008356: Provide a _entity_list route default to replace Controller\EntityListController and mimic _entity_form. It will fail, I'll just retest when that's in.

Status: Needs review » Needs work

The last submitted patch, menu-1984702-24.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
19.61 KB

Rerolling around #1999398: Use Symfony Request for menu module.
Will still fail.

Status: Needs review » Needs work

The last submitted patch, menu-1984702-26.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
18.97 KB

That went in.

dawehner’s picture

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.phpundefined
@@ -67,7 +106,27 @@ public function form(array $form, array &$form_state) {
+    $custom_exists = entity_load('menu', $value);

I guess we should inject the entity loading as well.

tim.plunkett’s picture

FileSize
22.37 KB
36.05 KB

That and the rest of the form too.

Status: Needs review » Needs work

The last submitted patch, menu-1984702-30.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
36.09 KB

Status: Needs review » Needs work

The last submitted patch, menu-1984702-32.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
932 bytes
36.1 KB

Bad variable name.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Great!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.phpundefined
@@ -111,11 +176,198 @@ public function save(array $form, array &$form_state) {
+    $form = &drupal_static(__FUNCTION__, array('#tree' => TRUE));

I think we should remove this static now we're OO :)

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.29 KB
36.68 KB

Absolutely.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

WFM.

alexpott’s picture

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

Needs a reroll...

git applyc https://drupal.org/files/menu-1984702-37.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 37564  100 37564    0     0  15837      0  0:00:02  0:00:02 --:--:-- 20560
error: patch failed: core/modules/menu/lib/Drupal/menu/MenuFormController.php:7
error: core/modules/menu/lib/Drupal/menu/MenuFormController.php: patch does not apply
tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Rerolling. Found some fun bugs in the process.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll, -WSCCI-conversion, -MENU_LOCAL_ACTION
FileSize
37.58 KB
44.02 KB

This contains #2027183: hook_menu() title callback is ignored on routes.
Posting a do-not-test patch without that part.

tim.plunkett’s picture

Blergh.

YesCT’s picture

This issue is taking out the unneeded $operation in the constructor of the MenuFormController that was put in in #1945226-143: Add language selector on menus
Related to:
#2027335: If EntityFormController implements EntityControllerInterface it can ignore $operation in createInstance()/__construct()

tim.plunkett’s picture

FileSize
37.57 KB

That went in, rerolled.

Status: Needs review » Needs work
Issue tags: -FormInterface, -WSCCI-conversion, -MENU_LOCAL_ACTION

The last submitted patch, menu-1984702-44.patch, failed testing.

tim.plunkett’s picture

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

#44: menu-1984702-44.patch queued for re-testing.

larowlan’s picture

+++ b/core/modules/menu/lib/Drupal/menu/Controller/MenuController.phpundefined
@@ -0,0 +1,100 @@
+   * @var \Drupal\Core\Entity\EntityStorageControllerInterface
...
+   * @param \Drupal\Core\Entity\EntityStorageControllerInterface $storage_controller
...
+  public function __construct(EntityStorageControllerInterface $storage_controller, EntityManager $entity_manager) {

should this be MenuLinkStorageControllerInterface now?

+++ b/core/modules/menu/lib/Drupal/menu/Controller/MenuController.phpundefined
@@ -0,0 +1,100 @@
+    $options = _menu_get_options(menu_get_menus(), $available_menus, array('mlid' => 0));

Should we add a todo to remove these once those two are part of a menu manager service? And add the follow up issue.

+++ b/core/modules/menu/lib/Drupal/menu/Controller/MenuController.phpundefined
@@ -0,0 +1,100 @@
+    drupal_set_title(t('Add menu link'));

Should we add a todo to remove when we've resolved title/title callback issue?

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.phpundefined
@@ -200,11 +250,201 @@ public function save(array $form, array &$form_state) {
+    global $menu_admin;

out of scope:oh yuck, that surely belongs in a menu service too

+++ b/core/modules/system/lib/Drupal/system/Tests/Menu/BreadcrumbTest.phpundefined
@@ -114,7 +114,6 @@ function testBreadCrumbs() {
-    $this->assertBreadcrumb('admin/structure/menu/manage/tools/edit', $trail);

Why is this removed?

tim.plunkett’s picture

FileSize
2.31 KB
37.79 KB

I opened the follow-up #2028249: Add a MenuManager service
But I disagree with linking that one inline, it could be a whole issue just to identify all the places to mark up.

I did add an @todo for the drupal_set_title().
Fixed the typehint, and renamed the variable in anticipation of #1976158: Rename entity storage/list/form/render "controllers" to handlers

That was removed because /edit is no longer a valid URL.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

unless bot says otherwise

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs a reroll..

git ac https://drupal.org/files/menu-1984702-48.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 38701  100 38701    0     0   9008      0  0:00:04  0:00:04 --:--:-- 13096
error: patch failed: core/modules/menu/menu.module:142
error: core/modules/menu/menu.module: patch does not apply
tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
37.69 KB

The getOperations issue added an access controller right where I was adding form controllers in the entity_info, so it conflicted. No changes.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the rerole.

alexpott’s picture

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

Needs a reroll

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
37.42 KB
YesCT’s picture

Issue tags: +RTBC July 1

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

tim.plunkett’s picture

FileSize
611 bytes
37.43 KB

Rerolled for loadMultiple. Leaving RTBC.

tim.plunkett’s picture

Rerolled for #849624: wrong permission for admin/structure/menu/parents.
Just update the routing.yml to match.

tim.plunkett’s picture

Yes, thank you d.o, I definitely wanted to kill those tags...

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
10.01 KB
45.96 KB

Well this was interesting. Major bugs in our new local tasks/actions.

Status: Needs review » Needs work

The last submitted patch, menu-1984702-59.patch, failed testing.

dawehner’s picture

Before we can even start with using parameters we need #2031487: When replacing the upcasted values in the request attributes array, retain the original raw value too to get in, because otherwise it will be full of upcasted stuff,
which will just produce a lot of brokenness all over the place.

tim.plunkett’s picture

Yes, I agree that is very much needed. My workarounds here are interesting but overkill.

dawehner’s picture

So what about convert them not yet to the new local action api? This lets the patch stay way more focused.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
37.45 KB

Fine by me.

dawehner’s picture

Thank you!

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.phpundefined
@@ -83,17 +140,36 @@ public function form(array $form, array &$form_state) {
+    $custom_exists = $this->entityQueryFactory->get('menu')->condition('id', $value)->range(0, 1)->count()->execute();
...
+    $link_exists = $this->entityQueryFactory->get('menu_link')->condition('menu_name', 'menu-' . $value)->range(0, 1)->count()->execute();

Bam! Potentially you could drop the second query if the first already returned something useful ...

tim.plunkett’s picture

FileSize
5.28 KB
38.63 KB

Good idea!
I also restored a test that was deleted, I don't know how that happened.

Also switched from using _permission and custom access checkers to the entity access checkers.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Nice! The issue is a classical example of what happens if you convert way too much at the same time.

tim.plunkett’s picture

This reuploads #66, but because it fails for me locally, trying a fix.
If they both pass, I'll be sad.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I'm sad on you're behalf... as I wouldn't know which patch is rtbc :)

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
38.3 KB

Reuploading #66 again by itself, the interdiff from #68 doesn't do anything and I have no idea why I did that.

alexpott’s picture

Title: Convert menu.module's page callbacks to Controllers » Change notice: Convert menu.module's page callbacks to Controllers
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed c5f81fb and pushed to 8.x. Thanks!

I think we need a change notice here because we've moved some function that menu manipulating modules in contrib might rely on.

andypost’s picture

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

I'm told that no one will volunteer to write a change notice if the issue is assigned, and there's a sprint coming up.
Feel free to ping me if any questions arise.

star-szr’s picture

Issue summary: View changes
Issue tags: +DrupalWorkParty

Tagging to write up the change record. Maybe we can start with before/after lists of functions and where they've moved to.

xjm’s picture

Issue tags: +Missing change record

Tagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release

It sounds like there's possibly two separate tasks here: adding the callbacks to https://drupal.org/node/2119699, and possibly a separate change record to document other menu.module API changes.

Gábor Hojtsy’s picture

Looking at this patch, I'm not sure why this needs a change notice for "menu module API changes". There are no **API functions** affected by this patch, which is shown clear by the fact that only the page callback related stuff (routing.yml, page callback file, controller) is affected.

I see the idea behind https://drupal.org/node/2119699, but to adapt that to all page controller patches, we would need to reopen several dozens of issues for change notices. We did not document one-on-one how other page callbacks were moved to classes either, that change notice only mentions one of them, although it appears it was intended to broadly cover the whole thing.

xjm’s picture

@Gábor Hojtsy, we don't need to reopen them -- just go down the list on the WSCCI meta, which I'm planning to ask some new contributors to work on in upcoming weeks. :) But if #71 is incorrect, and there are no menu module API changes, let's just add to the page callback list then and close this?

Gábor Hojtsy’s picture

Title: Change notice: Convert menu.module's page callbacks to Controllers » Convert menu.module's page callbacks to Controllers
Status: Active » Fixed
Issue tags: -Needs change record, -Missing change record

Ok, so we are not tracking that. That may be counter-productive if we want to get it done (we have a hard time getting done what we track). However, that is not related to *this* issue, so no need to discuss it here.

Added the page callback changes from this patch to https://drupal.org/node/2119699 and since there is no API change to document here otherwise AFAIS, closing.

webchick’s picture

Hm. I'm not really down with https://drupal.org/node/2119699 the way it is worded. We definitely do not want to waste anyone's time (let alone dozens of anyone's time) going through all of the WSCCI conversions and finding out what functions changed to what classes/methods, IMO. There is next to no reason that a module would ever call, say, menu_overview_page() directly. (And if there is, they can always look up the path in the routing file and figure out where it went.)

However, the title "Page callbacks converted to controllers" definitely deserves merit as a change notice; just the body should explain to module developers how to do this, using a D7 vs. D8 code example. This is indeed not a problem for this issue (or any other individual conversion issue) to solve, but is rather for #1971384: [META] Convert page callbacks to controllers.

Gábor Hojtsy’s picture

Question is if that would be a change notice or not. The WSCCI converison guide (several pages) exists to guide developers though that process: https://drupal.org/node/1953342

Crell’s picture

There's already a massive change notice for the routing system. (https://drupal.org/node/1800686) Probably too massive. And then the docs are in progress, and more complete than any change notice should be. Add a link to the docs to the existing routing change notice and call it a day. Listing individual callback->controller renames is pointless.

To expect people to learn how to upgrade their modules from D7 to D8 just from change notices is a fool's errand. Change notices are simply not suited to the level of change we're talking about here.

Status: Fixed » Closed (fixed)

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