Convert this page callback to a new-style Controller, using the instructions on http://drupal.org/node/1800686

Files: 
CommentFileSizeAuthor
#43 action-1939024-43.patch4.62 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 53,637 pass(es).
[ View ]
#43 interdiff.txt3.07 KBtim.plunkett
#41 action-1939024-41.patch1.56 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 53,735 pass(es), 14 fail(s), and 3 exception(s).
[ View ]
#31 drupal-action-manage-1939024-31.patch12.62 KBAlan Evans
PASSED: [[SimpleTest]]: [MySQL] 53,239 pass(es).
[ View ]
#31 interdiff.txt1.17 KBAlan Evans
#25 interdiff.txt4.11 KBAlan Evans
#25 drupal-action-manage-1939024-25.patch12.59 KBAlan Evans
PASSED: [[SimpleTest]]: [MySQL] 53,208 pass(es).
[ View ]
#22 drupal-action-manage-1939024-22.patch12.35 KBAlan Evans
PASSED: [[SimpleTest]]: [MySQL] 53,154 pass(es).
[ View ]
#22 interdiff.txt1.1 KBAlan Evans
#19 interdiff.txt1.87 KBAlan Evans
#19 drupal-action-manage-1939024-19.patch12.07 KBAlan Evans
PASSED: [[SimpleTest]]: [MySQL] 53,214 pass(es).
[ View ]
#13 interdiff.txt5.08 KBAlan Evans
#13 drupal-action-manage-1939024-13.patch11.83 KBAlan Evans
FAILED: [[SimpleTest]]: [MySQL] 53,136 pass(es), 14 fail(s), and 3 exception(s).
[ View ]
#7 drupal-action-manage-1939024-7.patch3.14 KBmtift
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-action-manage-1939024-7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#5 drupal-action-manage-1939024-5.patch9.91 KBAlan Evans
PASSED: [[SimpleTest]]: [MySQL] 53,105 pass(es).
[ View ]
#2 drupal-action-manage-1939024-2.patch6.44 KBAlan Evans
PASSED: [[SimpleTest]]: [MySQL] 53,076 pass(es).
[ View ]

Comments

Alan Evans’s picture

Looking into this - seems it would need to be merged with #1939026: Convert action_admin_remove_orphans to a new-style Controller at some point.

Alan Evans’s picture

StatusFileSize
new6.44 KB
PASSED: [[SimpleTest]]: [MySQL] 53,076 pass(es).
[ View ]

Attaching a patch for status/progress update - it works, but I want to convert the form also (which for one thing avoids using that module_load_include in this patch).

Alan Evans’s picture

Status:Active» Needs review
Alan Evans’s picture

Status:Needs review» Needs work
Alan Evans’s picture

Status:Needs work» Needs review
StatusFileSize
new9.91 KB
PASSED: [[SimpleTest]]: [MySQL] 53,105 pass(es).
[ View ]

Added form conversion, confirmed form does what it should ... a little unsure on some naming conventions.

Crell’s picture

Status:Needs review» Needs work
Issue tags:+FormInterface
+++ b/core/modules/action/lib/Drupal/action/Controller/ActionController.php
@@ -0,0 +1,92 @@
+<?php
+
+namespace Drupal\action\Controller;

Needs an @file docblock.

+++ b/core/modules/action/lib/Drupal/action/Controller/ActionController.php
@@ -0,0 +1,92 @@
+  public static function create(ContainerInterface $container) {
+    return new static();
+  }

Because this controller uses the database, we should inject the database connection here, save it to $this->database, and then modify the code below to not use db_query() but use $this->database->query(). (And similar.)

+++ b/core/modules/action/lib/Drupal/action/Controller/ActionController.php
@@ -0,0 +1,92 @@
+      $build['action_admin_manage_form'] = drupal_get_form(new \Drupal\action\ActionAdminManageForm(), $options);

Drupal coding standards say to always "use" the class at the top of the file, and then just use its short name here (ActionAdminManageForm).

Otherwise, I think this seems reasonable.

mtift’s picture

Status:Needs work» Needs review
StatusFileSize
new3.14 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-action-manage-1939024-7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I merged this patch with #1939026: Convert action_admin_remove_orphans to a new-style Controller, incorporated Crell's comments from both issues, and closed 1939026.

The one piece I did not understand was what it means to "inject the database connection." I couldn't find any other uses of "$this->database", but if you could point me to an example or documentation I would gladly look into it.

Status:Needs review» Needs work

The last submitted patch, drupal-action-manage-1939024-7.patch, failed testing.

mtift’s picture

Status:Needs work» Needs review

Sorry for not including the interdiff.txt. I made my changes before I committed the original patches.

Alan Evans’s picture

@Crell - thanks for the review! I was hoping someone would point me in the right direction on the database injection thing.

@mtift - thanks for working on this, but there seems to be a lot of changes missing from your patch... almost looks like you only posted an interdiff, and not the original. I'll look into merging everything together again including both our changes.

mtift’s picture

Alan, you are correct about the interdiff -- I was wondering why that patch failed so badly. I was trying to be helpful, and it was a good learning experience for me, so hopefully you can still just jump back in where you were. :)

Alan Evans’s picture

Yep, glad of the help, thanks.

The patch applies cleanly to where I was, so your changes have been incorporated and I'm just planning to put in Crell's database suggestions before uploading a new one.

Alan Evans’s picture

StatusFileSize
new11.83 KB
FAILED: [[SimpleTest]]: [MySQL] 53,136 pass(es), 14 fail(s), and 3 exception(s).
[ View ]
new5.08 KB

Here's where I'm at - I don't think it's 100% done still. Current patch contains:

  • the previous,
  • plus the remove-orphans issue
  • plus post-review changes from mtift
  • plus tweaks to get that all working
  • plus tweaks to stop it blowing up on me (see below)
  • plus the database change as suggested

Couple of problems remaining:

  1. It blew up on me ... for no reason that I can see. The same patch as was working above suddenly started throwing an exception on the action admin page, and it appears that the problem was that I'd left the old page callback still in place one level up in the menu (admin/config/system/actions). I solved this by replacing that level of the menu with the converted route also
  2. Because of the previous issue, I essentially need 2 paths pointing to the same route and I don't know how to add multiple values to the .yml file - I don't know how to add separate patterns for the base path, and the default local task ... though looking at this now, I fail to understand why this was implemented as a single local task in the first place - there's never a second tab.

Long story short - I think this patch still needs a little tweaking to resolve this local task issue. Any input on the multiple paths for a single item appreciated. I'll take a look when I can if the single default local task had any visible effect in the previous version.

Alan Evans’s picture

And I will fix that newline dammit! ... next iteration ...

Alan Evans’s picture

Maybe this is the way to handle that default argument on the local task:

blog:
pattern: /blog/{page}
defaults: { _controller: AcmeBlogBundle:Blog:index, page: 1 }

By adding page to the defaults key, the {page} placeholder is no longer required. The URL /blog will match this route and the value of the page parameter will be set to 1. The URL /blog/2 will also match, giving the page parameter a value of 2. Perfect.

Alan Evans’s picture

Reviewed the functionality of the page in vanilla 8.x - that local task doesn't create a tab at all, so I really have no idea what it's doing there. I'd propose just removing the lower level (the default local task) on that and be done.

Status:Needs review» Needs work

The last submitted patch, drupal-action-manage-1939024-13.patch, failed testing.

Alan Evans’s picture

The above test fails are reasonable, because the path the tests are using is no longer correct, though clicking through you'd never see that.

Not sure why I didn't see this patch blow up earlier - I can only imagine I was typing in urls manually instead of clicking around. The tests have the path hardcoded, so they naturally blow up when it changes, and the tests probably don't notice if the higher path (without /manage) fails.

I'm not keen on changing the path as part of this, so I guess make 2 routes pointing to the same place?

Alan Evans’s picture

StatusFileSize
new12.07 KB
PASSED: [[SimpleTest]]: [MySQL] 53,214 pass(es).
[ View ]
new1.87 KB

Progress ... should be all green now (at least it is locally - fingers crossed).

Alan Evans’s picture

Status:Needs work» Needs review
Crell’s picture

Status:Needs review» Needs work
+++ b/core/modules/action/lib/Drupal/action/Controller/ActionController.php
@@ -0,0 +1,114 @@
+  protected $database;

Needs a docblock with a @var delcaration for the class itself. That way IDEs can auto-complete off of it.

+++ b/core/modules/action/lib/Drupal/action/Controller/ActionController.php
@@ -0,0 +1,114 @@
+  public function __construct(Connection $database) {

Needs a docblock, even if it's a trivial one. "Constructs a new ActionController" (plus @param) is sufficient.

Otherwise, if the menu is still working I think we're good here. Thanks, you two! Let's tidy up the docs and get this put to bed.

Alan Evans’s picture

Status:Needs work» Needs review
StatusFileSize
new1.1 KB
new12.35 KB
PASSED: [[SimpleTest]]: [MySQL] 53,154 pass(es).
[ View ]

Thanks for the review and for bearing with me! Added some more docs ...

Would like to do one last manual runthrough to check it all works as it should (I was seeing a little disconnect between test results and real life results at the beginning of this thread), but I'll have to leave that for another day.

tim.plunkett’s picture

+++ b/core/modules/action/lib/Drupal/action/ActionAdminManageForm.phpundefined
@@ -0,0 +1,66 @@
+ * Configuration form for configurable actions.

Provides a configuration form....

+++ b/core/modules/action/lib/Drupal/action/ActionAdminManageForm.phpundefined
@@ -0,0 +1,66 @@
+   * @param array $form
+   * @param array $form_state

Delete these lines

+++ b/core/modules/action/lib/Drupal/action/ActionAdminManageForm.phpundefined
@@ -0,0 +1,66 @@
+      '#attributes' => array('class' => array('container-inline')),
...
+    $form['parent']['actions'] = array('#type' => 'actions');

These are usually split over multiple lines.

+++ b/core/modules/action/lib/Drupal/action/ActionAdminManageForm.phpundefined
@@ -0,0 +1,66 @@
+  public function validateForm(array &$form, array &$form_state) { }

Closing } goes on a new line.

+++ b/core/modules/action/lib/Drupal/action/ActionAdminManageForm.phpundefined
@@ -0,0 +1,66 @@
+    }
+  }
+}
diff --git a/core/modules/action/lib/Drupal/action/Controller/ActionController.php b/core/modules/action/lib/Drupal/action/Controller/ActionController.php

Missing a blank line after the method before the end of the class.

+++ b/core/modules/action/lib/Drupal/action/Controller/ActionController.phpundefined
@@ -0,0 +1,127 @@
+  public static function create(ContainerInterface $container) {
...
+  public function __construct(Connection $database) {

__construct should be the first method in the class.

+++ b/core/modules/action/lib/Drupal/action/Controller/ActionController.phpundefined
@@ -0,0 +1,127 @@
+   * @param \Drupal\Core\Database\Connection $database

Missing the description line

+++ b/core/modules/action/lib/Drupal/action/Controller/ActionController.phpundefined
@@ -0,0 +1,127 @@
+   * Displays an overview of available and configured actions.
+   */
+  public function adminManage() {

Missing @return

+++ b/core/modules/action/lib/Drupal/action/Controller/ActionController.phpundefined
@@ -0,0 +1,127 @@
+      $build['action_table'] = array('#markup' => theme('table', array('header' => $header, 'rows' => $rows)));

I realize this is probably moved code, but it should be

$build['action_table'] = array(
  '#theme' => 'table',
  '#header' => $header,
  '#rows' => $rows,
);
+++ b/core/modules/action/lib/Drupal/action/Controller/ActionController.phpundefined
@@ -0,0 +1,127 @@
+    return new RedirectResponse(url('admin/config/system/actions'));

I believe you need array('absolute' => TRUE) passed for the options to url(). Does this have test coverage?

Alan Evans’s picture

Thanks for the thorough review, Tim, and sorry for the careless style errors.

btw - the manual test run that I was planning turned out fine.

Looks like you're right btw about the absolute url for redirects: while Chrome does something sensible with the "relative" url, the Location header is not strictly correct/ backward compatible.

In general, test coverage for action seems reasonable, but my initial patches were passing tests where manual testing revealed issues, so possibly not sufficient. The discrepancy was caused by existing tests relying on hardcoded paths (rather than a user behaviour), meaning 1) They don't uncover issues that occur at an equivalent path to the hardcoded one and 2) They are anchored to those paths and blow up if the path has to change.

Should have corrections shortly ...

Alan Evans’s picture

StatusFileSize
new12.59 KB
PASSED: [[SimpleTest]]: [MySQL] 53,208 pass(es).
[ View ]
new4.11 KB
mtift’s picture

+++ b/core/modules/action/lib/Drupal/action/ActionAdminManageForm.phpundefined
@@ -0,0 +1,68 @@
+ * @file
+ * Contains \Drupal\action\ActionAdminManageForm.
+ */
+
+namespace Drupal\action;
+

I'm looking at http://drupal.org/node/1800686 and #1945564: Convert confirm_form() in shortcut to the new form interface and convert route and wondering if the namespace (and directory structure) here should be Drupal\action\Form?

mtift’s picture

I also did not find any information about Forms on this page (under development) "PSR-0 path naming conventions": http://drupal.org/node/1929784, and it seems like that would be the page for it.

Crell’s picture

Status:Needs review» Needs work

I don't think we have a formal standard yet. I've been encouraging Drupal\$module\Form\SomethingForm and Drupal\$module\Controller\ThingieController, but that's not a hard rule at present.

I guess we should probably switch to that, and then I'll let Tim have final word on this one.

tim.plunkett’s picture

Yes please, to both Form\SomethingForm and Controller\SomethingController

Alan Evans’s picture

Makes sense ... on it.

Alan Evans’s picture

StatusFileSize
new1.17 KB
new12.62 KB
PASSED: [[SimpleTest]]: [MySQL] 53,239 pass(es).
[ View ]

Note the interdiff only shows the text modifications, not the file move.

Alan Evans’s picture

Status:Needs work» Needs review

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

The last submitted patch, drupal-action-manage-1939024-31.patch, failed testing.

Alan Evans’s picture

Status:Needs work» Needs review
Issue tags:+WSCCI, +FormInterface, +WSCCI-conversion
mtift’s picture

I want to try and document a discussion with @msonnabaum in the WSCCI IRC meeting yesterday regarding this patch in particular, but also conversion patches generally. @Alan Evans, sorry to pile on in your issue, but I think the suggestions below might have much broader implications and I wasn't sure if there was a better place to post this information.

@msonnabaum was advocating that PHPUnit tests be part of the conversion issues. His feeling was that we could encourage it, but not require it. He said that "if a new patch comes in adding a class, and it either a) doesn't have tests or b) has tests, but uses UnitTestBase, we should encourage using PHPUnit." He said that the tests in core/tests could serve as examples.

Further, with reference to this particular patch, he thought that adding a procedural function, such as action_synchronize(), in a new OOP class will make testing more difficult.

Crell’s picture

While I agree with mark on PHPUnit++ and avoiding function calls in classes, for controller conversion I don't know that it's the appropriate place to really drive that. action_synchronize() is an existing function. It really ought to turn into an action management service, a la BookManager in #1938296: Convert book_admin_overview and book_render to a new-style Controller, but I don't know that we can make that a requirement for the conversion issues themselves as that increases the barrier to entry for these patches. I'd almost prefer to mostly not do that for now (with some exceptions), and then come back in a second pass to make more module services by eliminating functions. That way more people will have had their hands in the pie, and gotten experience with OO and why dependencies are nice.

I'd rather not throw 3 new things at people at once (controllers, PHPUnit, service conversion), when we finally after 2 years have something in WSCCI we can crowdsource. If someone wants to I certainly won't stop them, but I don't think we can make that a requirement.

mtift’s picture

So it sounds like everyone basically agrees and that 31 is ready to go. I'd mark this RTBC, but I don't quite feel qualified to do that, yet. :)

msonnabaum’s picture

Looked at this patch a bit closer. The only thing worth unit testing is ActionController::adminManage. It's already too long as it is, so it would benefit from being decomposed a bit so the smaller methods could be tested, but it's using procedural functions throughout, so it's probably not worth it here.

Crell’s picture

Status:Needs review» Reviewed & tested by the community

Then let's move forward and do further refactoring later. Thanks, Mark.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

tim.plunkett’s picture

Status:Fixed» Needs review
StatusFileSize
new1.56 KB
FAILED: [[SimpleTest]]: [MySQL] 53,735 pass(es), 14 fail(s), and 3 exception(s).
[ View ]

Quick follow-up, a hook_menu() entry is not needed for MENU_CALLBACK once converted, and route_name is redundant with MENU_DEFAULT_LOCAL_TASK. You'll notice the removed route is identical to the one above it.

Status:Needs review» Needs work

The last submitted patch, action-1939024-41.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new3.07 KB
new4.62 KB
PASSED: [[SimpleTest]]: [MySQL] 53,637 pass(es).
[ View ]

Ah, didn't notice that other code was incorrectly directly addressing the default path.

Crell’s picture

Status:Needs review» Reviewed & tested by the community

You are correct sir!

andypost’s picture

+1 to commit

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

Status:Fixed» Closed (fixed)

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

kim.pepper’s picture

I think was something missed in the conversion. We still have this in action.admin.inc that no longer gets called:

<?php
/**
 * Post-deletion operations for deleting action orphans.
 *
 * @param $orphaned
 *   An array of orphaned actions.
 */
function action_admin_delete_orphans_post($orphaned) {
  foreach (
$orphaned as $callback) {
   
drupal_set_message(t("Deleted orphaned action (%action).", array('%action' => $callback)));
  }
}
?>
Alan Evans’s picture

I've tried quickly rewinding my local checkout to ef63cffdf3dc2a873 and seeing what might have previously called this and I still see no mention of either action_admin_delete_orphans_post or even just action_admin_delete_orphans (assuming that the _post may have been dynamically added). It appears that it was also not directly mentioned in code prior to this patch (or more likely I haven't found it yet).

Have you checked manually already whether this was called previously called and no longer is, or just grepped? Do you know where it was previously called from?

andypost’s picture

Status:Closed (fixed)» Active
Issue tags:+Needs manual testing

Actions have bad test coverage so needs manual testing #1412964: Add additional test coverage for actions and probably follow-up

tim.plunkett’s picture

Status:Active» Closed (fixed)

This was fixed, reclosing.