Convert these page callbacks to new-style Controllers, using the instructions on http://drupal.org/node/1800686

Files: 
CommentFileSizeAuthor
#34 ban-1944910-34.patch14.33 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,995 pass(es).
[ View ]
#34 interdiff.txt3.6 KBtim.plunkett
#31 interdiff.txt10.18 KBtim.plunkett
#31 ban-1944910-31.patch14.33 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 56,029 pass(es).
[ View ]
#21 ban-1944910-21.patch12.47 KBbarraponto
PASSED: [[SimpleTest]]: [MySQL] 55,973 pass(es).
[ View ]
#20 ban-1944910-20.patch13.45 KBbarraponto
FAILED: [[SimpleTest]]: [MySQL] 55,770 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#17 ban-1944910-17.patch12.47 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,693 pass(es).
[ View ]
#17 interdiff.txt931 bytestim.plunkett
#16 interdiff.txt530 bytesjibran
#16 ban-1944910-16.patch12.46 KBjibran
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#14 ban-1944910-14.patch12.46 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,966 pass(es).
[ View ]
#14 interdiff.txt2.4 KBtim.plunkett
#12 ban-1944910-12.patch12.45 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 54,394 pass(es), 2 fail(s), and 15 exception(s).
[ View ]
#10 ban-1944910-10.patch22.11 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch ban-1944910-10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#10 interdiff.txt5.38 KBtim.plunkett
#9 ban-1944910-9.patch11.04 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 54,182 pass(es).
[ View ]
#8 ban-1944910-8.patch16.01 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 54,086 pass(es).
[ View ]
#2 ban-1944910-2.patch11.04 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 53,622 pass(es), 4 fail(s), and 1 exception(s).
[ View ]
#2 ban-1944910-2-hacky.patch11.38 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 53,289 pass(es).
[ View ]
#2 interdiff.txt947 bytestim.plunkett

Comments

Assigned:Unassigned» tim.plunkett

Working on this, since it will be the first new example of #1938600: Add a FormInterface replacement for confirm_form() as well.

Status:Active» Needs review
StatusFileSize
new947 bytes
new11.38 KB
PASSED: [[SimpleTest]]: [MySQL] 53,289 pass(es).
[ View ]
new11.04 KB
FAILED: [[SimpleTest]]: [MySQL] 53,622 pass(es), 4 fail(s), and 1 exception(s).
[ View ]

Okay, so I clearly am either missing something with how routes work with breadcrumbs and menu trees, or this is busted.
Here's two versions of the patch, one as I thought it should be, and one that is hacky but works.

+++ b/core/modules/ban/lib/Drupal/ban/Form/BanDelete.phpundefined
@@ -0,0 +1,76 @@
+   * @param int $ban_ip_id
+   *   The IP address record ID to unban.
+   */
+  public function buildForm(array $form, array &$form_state, $ban_ip = '') {

The parameter is optional, but not marked as such. What happens when it's empty?

Also, the docs say $ban_ip_id is an int, but you have a thing called $ban_ip that defaults to an empty string, but the protected property $banIP is an empty array...

+++ b/core/modules/ban/ban.routing.yml
@@ -0,0 +1,14 @@
+  defaults:
+    _form: '\Drupal\ban\Form\BanDelete'

You don't have a default for ban_ip listed here, but the buildForm() method lists it as optional.

For routing purposes, default values in the signature of the controller mean diddlysquat. The routing system looks only at the route definition. Really, optional parameters in the controller signature itself are just a confusion at this point. (It has to do that, because that's how /foo/bar and /foo/bar/{baz} can be differentiated before the controllers are known.)

Status:Needs review» Needs work

Status:Needs work» Needs review

#2: ban-1944910-2.patch queued for re-testing.

Status:Needs review» Postponed

Status:Postponed» Needs review
StatusFileSize
new16.01 KB
PASSED: [[SimpleTest]]: [MySQL] 54,086 pass(es).
[ View ]

Okay, just to test the patch from the other issue, here's the failing patch from #2 combined with it.

StatusFileSize
new11.04 KB
PASSED: [[SimpleTest]]: [MySQL] 54,182 pass(es).
[ View ]

That went in, reroll.

StatusFileSize
new5.38 KB
new22.11 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch ban-1944910-10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled, and injected all the things!

Status:Needs review» Needs work

The last submitted patch, ban-1944910-10.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new12.45 KB
FAILED: [[SimpleTest]]: [MySQL] 54,394 pass(es), 2 fail(s), and 15 exception(s).
[ View ]

Didn't rebase all the way.

Status:Needs review» Needs work

The last submitted patch, ban-1944910-12.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.4 KB
new12.46 KB
PASSED: [[SimpleTest]]: [MySQL] 55,966 pass(es).
[ View ]

UGH I keep doing that. Wrong class name.

+++ b/core/modules/ban/lib/Drupal/ban/Form/BanDelete.phpundefined
@@ -0,0 +1,104 @@
+   * @param int $ban_ip_id
...
+  public function buildForm(array $form, array &$form_state, $ban_ip = '') {

I guess it's a string stored in the DB, right?

StatusFileSize
new12.46 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new530 bytes

Bump and fixed #15.

StatusFileSize
new931 bytes
new12.47 KB
PASSED: [[SimpleTest]]: [MySQL] 55,693 pass(es).
[ View ]

I think @dawehner meant the opposite.

Status:Needs review» Needs work

+++ b/core/modules/ban/lib/Drupal/ban/Form/BanAdmin.phpundefined
@@ -0,0 +1,145 @@
+   * The request object.
+   *
+   * @var \Symfony\Component\HttpFoundation\Request
+   */
+  protected $request;
...
+  public function __construct(Request $request, Connection $database) {
...
+      $container->get('request'),

no need to inject request, add it as an argument to buildForm() and then store it in $form_state so it can be reused in validator
see http://drupal.org/node/1972260 as example

+++ b/core/modules/ban/lib/Drupal/ban/Form/BanAdmin.phpundefined
@@ -0,0 +1,145 @@
+   * {@inheritdoc}
+   *
+   * @param string $default_ip
+   *   (optional) IP address to be passed on to drupal_get_form() for use as the
+   *   default value of the IP address form field.
+++ b/core/modules/ban/lib/Drupal/ban/Form/BanDelete.phpundefined
@@ -0,0 +1,104 @@
+   * {@inheritdoc}
...
+   * @param string $ban_ip_id
+   *   The IP address record ID to unban.

wont work. either old style, Overrides... or no additional comment. it is being parsed only when alone

+++ b/core/modules/ban/lib/Drupal/ban/Form/BanDelete.phpundefined
@@ -0,0 +1,104 @@
+   * Constructs a new BanAdmin object.

new BanDelete

About the inheritdoc, quoting @dawehner:

I do think this is okay: http://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutoria... ... so if this is a problem the api module should support that as well.

StatusFileSize
new13.45 KB
FAILED: [[SimpleTest]]: [MySQL] 55,770 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Rerolled, fixed third issue mentioned in #18. The second issue seems not to be an issue at all and the first one... well I'm clueless.

StatusFileSize
new12.47 KB
PASSED: [[SimpleTest]]: [MySQL] 55,973 pass(es).
[ View ]

My bad, core doesn't use format-patch...

Status:Needs work» Needs review

Invoking the bot.

Status:Needs review» Needs work

Just in general I'm wondering whether it is good to move direct database calls back to form instead of having some kind of CRUD api.

Good idea @dawehner. There is an BanIpManager already that does db calls. Let's move it there.

+++ b/core/modules/ban/lib/Drupal/ban/Form/BanAdmin.phpundefined
@@ -0,0 +1,145 @@
+    $header = array(t('banned IP addresses'), t('Operations'));

Can we move this to BanIpManager::findAll()

+++ b/core/modules/ban/lib/Drupal/ban/Form/BanAdmin.phpundefined
@@ -0,0 +1,145 @@
+    if ($this->database->query("SELECT * FROM {ban_ip} WHERE ip = :ip", array(':ip' => $ip))->fetchField()) {

Move this to BanIpManager::isBanned($ip)

+++ b/core/modules/ban/lib/Drupal/ban/Form/BanAdmin.phpundefined
@@ -0,0 +1,145 @@
+    $this->database->insert('ban_ip')
+      ->fields(array('ip' => $ip))

Move this to BanIpManager::addIp($ip)

+++ b/core/modules/ban/lib/Drupal/ban/Form/BanDelete.phpundefined
@@ -0,0 +1,104 @@
+    $this->banIP = $this->database->query("SELECT * FROM {ban_ip} WHERE iid = :iid", array(':iid' => $ban_ip_id))->fetchAssoc();

Move to BanIpManager::findById()

+++ b/core/modules/ban/lib/Drupal/ban/Form/BanDelete.phpundefined
@@ -0,0 +1,104 @@
+    $this->database->delete('ban_ip')
+      ->condition('iid', $this->banIP['iid'])
+      ->execute();

Move to BanIpManager::delete($iid)

Status:Needs work» Needs review

#21: ban-1944910-20.patch queued for re-testing.

Status:Needs review» Needs work

+++ b/core/modules/ban/lib/Drupal/ban/Form/BanAdmin.php
@@ -0,0 +1,145 @@
+  public function __construct(Request $request, Connection $database) {
+    $this->request = $request;
+    $this->database = $database;
+  }

The request should not be a constructor parameter. Instead, specify Request $request as an optional parameter to buildForm() and it will get passed in by the routing system there. You can then save it to a property if you need it for the validate and submit methods.

Agreed with adding a separate service object for handling the DB interaction. I'd suggest BanIpRepository as the name.

Can we use \Drupal::request() and mark it as a follow-up to pass it in when #1998166: Use the controller resolver to inject parameters into HtmlFormController is in?

@Crell BanIpRepository, eh? @msonnabaum would be so proud. :-)

@Crell. Just re-read the issue and we already have BanIpManager service that is doing db lookups. Suggest we use that, and file a separate issue to rename it to BanIpRepository.

Agreed on #29.

Status:Needs work» Needs review
StatusFileSize
new14.33 KB
PASSED: [[SimpleTest]]: [MySQL] 56,029 pass(es).
[ View ]
new10.18 KB

I went with banIP() and unbanIP() instead of addIP() and delete(), it seemed more intuitive.

+++ b/core/modules/ban/lib/Drupal/ban/BanIpManager.phpundefined
@@ -46,4 +46,65 @@ public function isDenied($ip) {
+  public function banIP($ip) {

I thought the naming convention would make this banIp() rather than banIP()?

+++ b/core/modules/ban/lib/Drupal/ban/BanIpManager.phpundefined
@@ -46,4 +46,65 @@ public function isDenied($ip) {
+  public function findByID($ban_id) {

Ibid

StatusFileSize
new3.6 KB
new14.33 KB
PASSED: [[SimpleTest]]: [MySQL] 55,995 pass(es).
[ View ]

Ugh I hate that standard. As @jhodgdon would say, "Ego, Id, Superego"

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

The last submitted patch, ban-1944910-34.patch, failed testing.

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

#34: ban-1944910-34.patch queued for re-testing.

Title:Convert the 2 menu callbacks in ban.module to a new style controller (and form controller)Convert ban.module's page callbacks to Controllers

Status:Needs review» Reviewed & tested by the community

Tim's all about the nearly-total module rewrites lately. :-)

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks.

FYI, our review of this patch led us to suggest removing one docs line from all .twig files - #2013094: [policy adopted, patch needed] Stop saying '@see template_preprocess()' in every twig file

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

Issue summary:View changes

Fixing link