Convert action_admin_manage to a new-style Controller

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.

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

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

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.

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

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

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

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.

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.

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

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.

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.

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?

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

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.

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

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

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

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.

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

Makes sense ... on it.

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

#31: drupal-action-manage-1939024-31.patch queued for re-testing.

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.

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

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.

Committed and pushed to 8.x. Thanks!

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.

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

+1 to commit

Committed and pushed to 8.x. Thanks!

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)));
  }
}
?>

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?

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

This was fixed, reclosing.