Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

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.

tim.plunkett’s picture

Status: Active » Needs review
FileSize
947 bytes
11.38 KB
11.04 KB

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.

xjm’s picture

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

Crell’s picture

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

tim.plunkett’s picture

Status: Needs review » Needs work
tim.plunkett’s picture

Status: Needs work » Needs review

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

tim.plunkett’s picture

Status: Needs review » Postponed
tim.plunkett’s picture

Status: Postponed » Needs review
FileSize
16.01 KB

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

tim.plunkett’s picture

FileSize
11.04 KB

That went in, reroll.

tim.plunkett’s picture

FileSize
5.38 KB
22.11 KB

Rerolled, and injected all the things!

Status: Needs review » Needs work

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
12.45 KB

Didn't rebase all the way.

Status: Needs review » Needs work

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
2.4 KB
12.46 KB

UGH I keep doing that. Wrong class name.

dawehner’s picture

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

jibran’s picture

FileSize
12.46 KB
530 bytes

Bump and fixed #15.

tim.plunkett’s picture

FileSize
931 bytes
12.47 KB

I think @dawehner meant the opposite.

ParisLiakos’s picture

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

tim.plunkett’s picture

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.

barraponto’s picture

FileSize
13.45 KB

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.

barraponto’s picture

FileSize
12.47 KB

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

kim.pepper’s picture

Status: Needs work » Needs review

Invoking the bot.

dawehner’s picture

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.

kim.pepper’s picture

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)

jibran’s picture

Status: Needs work » Needs review

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

Crell’s picture

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.

tim.plunkett’s picture

kim.pepper’s picture

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

kim.pepper’s picture

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

Crell’s picture

Agreed on #29.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
14.33 KB
10.18 KB

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

kim.pepper’s picture

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

kim.pepper’s picture

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

Ibid

tim.plunkett’s picture

FileSize
3.6 KB
14.33 KB

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.

tim.plunkett’s picture

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

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

tim.plunkett’s picture

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
Crell’s picture

Status: Needs review » Reviewed & tested by the community

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

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

moshe weitzman’s picture

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.

Anonymous’s picture

Issue summary: View changes

Fixing link