Proposed solution

#1570102: [Policy] Deprecate Ban module

original issue

Another spin-off from #679112: Time for system.module and most of includes to commit seppuku.

IP blocking is currently hardcoded into _drupal_bootstrap_page_cache() and system.module.

I know of very few sites that use the feature - most will either do this outside of Drupal altogether, or use something more featured like troll or bad behaviour.

Most of access rules was removed in Drupal 7 (see #228594: UMN Usability: split access rules into an optional module), but Dries wanted this particular feature left in. While it is tempting to post patches that just delete hundreds of lines of code, they don't always get in, so this one is a compromise.

Patch adds a new module, 'ban' (not block ;)). This provides the admin screen, and implements hook_boot().

There should be very, very little performance difference between an IP being banned from hook_boot() and from drupal_bootstrap_page_cache(). For IPs that aren't banned, there'll be no difference at all.

However where there is more of a performance difference in aggregate is on sites that have the module disabled. It is one less table to create during drupal_install_system() (which was leading to timeouts on some hosting providers), less in the schema cache, less code in bootstrap.inc etc. - we should aim to get this kind of overhead out of the critical path as much as possible.

We could also just remove it from core, if we did that it'd just be a case of removing all the lines from the patch with a + in front.

Patch is missing an upgrade path, that will need to:

* rename the database table
* rename the index
* rename the permission
* rename/change ownership of the action

Diffstat:

diffstat 1161486-ban_module-45.patch
 b/core/includes/bootstrap.inc                                                         |   51 ---
 b/core/modules/ban/ban.admin.inc                                                      |  115 +++++++
 b/core/modules/ban/ban.info                                                           |    6
 b/core/modules/ban/ban.install                                                        |   37 ++
 b/core/modules/ban/ban.module                                                         |  154 ++++++++++
 b/core/modules/ban/lib/Drupal/ban/Tests/IpAddressBlockingTest.php                     |   88 +++++
 b/core/modules/statistics/lib/Drupal/statistics/Tests/StatisticsBlockVisitorsTest.php |   32 +-
 b/core/modules/statistics/lib/Drupal/statistics/Tests/StatisticsTestBase.php          |    2
 b/core/modules/statistics/statistics.admin.inc                                        |   12
 b/core/modules/statistics/statistics.info                                             |    1
 b/core/modules/system/system.admin.inc                                                |  105 ------
 b/core/modules/system/system.install                                                  |   59 ++-
 b/core/modules/system/system.module                                                   |   54 ---
 core/modules/system/lib/Drupal/system/Tests/System/IpAddressBlockingTest.php          |   89 -----
 14 files changed, 460 insertions(+), 345 deletions(-)

catch@catch-laptop:~/www/8$ diffstat ban.patch
 includes/bootstrap.inc          |   50 -------------
 modules/ban/ban.admin.inc       |  112 ++++++++++++++++++++++++++++++
 modules/ban/ban.info            |    7 +
 modules/ban/ban.install         |   37 ++++++++++
 modules/ban/ban.module          |  146 ++++++++++++++++++++++++++++++++++++++++
 modules/ban/ban.test            |   84 +++++++++++++++++++++++
 modules/system/system.admin.inc |  104 ----------------------------
 modules/system/system.module    |   54 --------------
 modules/system/system.test      |   79 ---------------------
 9 files changed, 386 insertions(+), 287 deletions(-)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Status: Needs work » Needs review

Actually CNR for the bot to ensure ban.test passes and nothing else fails.

If this test passes, then it shows how much #1119094: Add a test to verify the schema matches after a database update is needed.

Crell’s picture

I am fully in support of this patch. I've not tried running it but on visual inspection it seems sane. This is definitely low-hanging fruit for detangling Drupal core.

Let's see what the testbot says about it, even though the upgrade path is missing.

catch’s picture

FileSize
26.21 KB

Patch was missing hunk from system.install


catch@catch-laptop:~/www/8$ diffstat ban.patch
 includes/bootstrap.inc          |   50 -------------
 modules/ban/ban.admin.inc       |  112 ++++++++++++++++++++++++++++++
 modules/ban/ban.info            |    7 +
 modules/ban/ban.install         |   37 ++++++++++
 modules/ban/ban.module          |  146 ++++++++++++++++++++++++++++++++++++++++
 modules/ban/ban.test            |   84 +++++++++++++++++++++++
 modules/system/system.admin.inc |  104 ----------------------------
 modules/system/system.install   |   23 ------
 modules/system/system.module    |   54 --------------
 modules/system/system.test      |   79 ---------------------
 10 files changed, 386 insertions(+), 310 deletions(-)
sun’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Framework Initiative, +API clean-up

Status: Reviewed & tested by the community » Needs work

The last submitted patch, ban.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
Issue tags: +API change
FileSize
32.19 KB

Statistics module needs to depend on ban module. It would be theoretically possible to move the functionality out of statistics module into ban module, but the logic is pretty tightly coupled (operations in tables etc.).

This patch should pass tests now I think, but it still cannot be marked RTBC because there is no upgrade path.

Adding API change tag since this technically is one.

Status: Needs review » Needs work

The last submitted patch, ban.patch, failed testing.

podarok’s picture

subscribe

catch’s picture

Status: Needs work » Needs review

#7: ban.patch queued for re-testing.

joachim’s picture

Status: Needs review » Needs work

> Statistics module needs to depend on ban module.

That's going to look like a WTF on D8 :/

+++ b/modules/statistics/statistics.admin.inc
@@ -101,7 +101,7 @@ function statistics_top_visitors() {
-  $query->leftJoin('blocked_ips', 'bl', 'a.hostname = bl.ip');
+  $query->leftJoin('banned_ips', 'b', 'a.hostname = b.ip');

Could you not just wrap these bits up in a module_exists() ?

Powered by Dreditor.

catch’s picture

@joachim: it's already a wtf, it's just one that is in drupal_bootstrap_configuration().

If you look at statistics.module, there is a lot more than one part of a db_select() to do with IP banning.

Leaving at needs work since there is still the upgrade path to do, but this passes tests now.

mirocow’s picture

subscribe

aspilicious’s picture

Good time for a doc cleanup. Still needs the upgrade path :)

marcingy’s picture

Status: Needs work » Needs review
FileSize
34.54 KB

Initial attempt at upgrade path.

marcingy’s picture

Status: Needs review » Needs work

Putting back to needs work as after some thinking about it I don't believe the actions change is correct instead we need to delete the dead action and update any triggers that are using the action instead of simply renaming. Will look at this tonight.

marcingy’s picture

Status: Needs work » Needs review
FileSize
34.72 KB

New version which renames exist trigger assignments for blocking ips and then cleans up the old action from the actions table.

marcingy’s picture

Status: Needs review » Needs work

We need an upgrade test path for this I'll try and work on this next, setting back to need work as the bot is happy with the patch but of course that totally circumvents the upgrade path.

andypost’s picture

Status: Needs work » Needs review
Issue tags: -Framework Initiative, -API change, -API clean-up

Status: Needs review » Needs work
Issue tags: +Framework Initiative, +API change, +API clean-up

The last submitted patch, 1161486-ban-module-doc-cleanup-17.patch, failed testing.

amateescu’s picture

Assigned: Unassigned » amateescu

I'll work on this in a few days. One issue that should be taken into consideration here is #1282156: Convert IP-blocking table to "Empty table pattern".

andypost’s picture

Status: Needs work » Needs review
FileSize
34.16 KB

here's re-roll with some fixes:
- #21
- removed ban_install and update moved to system_update_8014, without trigger tables
- changed strings in tests
- fixes tests

andypost’s picture

FileSize
34.52 KB

- fixed @file block of test
- changed assert messages do not use t()

RobLoach’s picture

Status: Needs review » Needs work

Oh, I like this a lot. It looks pretty close to RTBC as well!

+++ b/core/modules/ban/ban.installundefined
@@ -0,0 +1,37 @@
+  $schema['banned_ips'] = array(
+    'description' => 'Stores banned IP addresses.',
+    'fields' => array(
+       'iid' => array(
+        'description' => 'Primary Key: unique ID for IP addresses.',
+        'type' => 'serial',
+        'unsigned' => TRUE,
+        'not null' => TRUE,
+      ),
+      'ip' => array(
+        'description' => 'IP address',
+        'type' => 'varchar',

The "blocked_ips" schema should probably be removed from system.install since we're moving it over to ban module.

+++ b/core/modules/system/system.installundefined
@@ -1972,6 +1972,38 @@ function system_update_8013() {
+    // Delete old permissions.
+    db_delete('role_permission')
+      ->condition('permission', 'block IP addresses')
+      ->execute();
+
+    // Remove legacy action.
+    db_delete('actions')
+      ->condition('aid', 'system_block_ip_action')

It looks like we're not migrating over the permissions for block IP address, or the action configuration associated with it. Is this intentional? Could probably just update those rows using the same conditions.

Crell’s picture

andypost asked me to comment here in light of #1599108: Allow modules to register services and subscriber services (events).

In the Kernel world, the "proper" place for this to tie in would be in the kernel.request event. That event can set a 403 response object directly, which short circuits all further behavior. That will become possible for modules to do as soon as the above patch lands.

andypost’s picture

@Rob Loach thanx for review, permissions should be migrated but actions have no config becase it was stored in trigger module assignments. Action itself will be regusted but old one we need to delete.

As @Crell pointed we have to wait the patch to land

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
31.61 KB

Added a check to see if statistics are enabled during the update, since it is now a dependency, which i cant understand why (didnt search too much tbh)
It also updates permissions table. i am not sure what to do with actions here

Also removed $schema['blocked_ips'] form system.install per #24

Status: Needs review » Needs work

The last submitted patch, 1161486-ban_module-27.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
35.1 KB

Yeah forgot removing core/modules/system/lib/Drupal/system/Tests/System/IpAddressBlockingTest.php when applying manually, sorry for this

Status: Needs review » Needs work

The last submitted patch, 1161486-ban_module-28.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
35.27 KB

Ban module was not enabled during tests, had to do some changes there.
Also manually tested upgrade path and permissions are transferred successfully. Dunno if it is the correct way to do this

andypost’s picture

Fix tests and added removal of old test file

EDIT I'm not familiar with new way to add event listeners so let's get the module commited and fix it's hook_boot() implementation when new cache system landed

ParisLiakos’s picture

Merged in andypost's changes to keep things less confusing and attached interdiffs

andypost’s picture

Status: Needs review » Needs work
+++ b/core/modules/ban/lib/Drupal/ban/Tests/IpAddressBlockingTest.php
@@ -0,0 +1,94 @@
+  /**
+   * The main user for testing.
+   *
+   * @var object
+   */
+  protected $blocking_user;

not needed

+++ b/core/modules/ban/lib/Drupal/ban/Tests/IpAddressBlockingTest.php
@@ -0,0 +1,94 @@
+    $this->banning_user = $this->drupalCreateUser(array('ban IP addresses'));
+    $this->drupalLogin($this->banning_user);

Change to $admin_user

+++ b/core/modules/ban/lib/Drupal/ban/Tests/IpAddressBlockingTest.php
@@ -0,0 +1,94 @@
+    // Submit your own IP address. This fails, although it works when testing manually.

Wrap 'manually.' to another line

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
710 bytes
35.44 KB

Thanks, changed as above

Status: Needs review » Needs work

The last submitted patch, 1161486-ban_module-35.patch, failed testing.

andypost’s picture

Status: Needs work » Reviewed & tested by the community

Now it's good to go. Manually tested patch and bot was wrong

catch’s picture

Status: Reviewed & tested by the community » Needs work

This needs a general hook_help() for the module description to appear as admin/help. Should be a relatively simple copy/paste from one of the others for the format and a short description.

ParisLiakos’s picture

Status: Needs work » Reviewed & tested by the community

hmm it is there already:S

+
+/**
+ * Implements hook_help().
+ */
+function ban_help($path, $arg) {
+  switch ($path) {
+    case 'admin/config/people/ban':
+      return '<p>' . t('IP addresses listed here are banned from your site. Banned addresses are completely forbidden from accessing the site and instead see a brief message explaining the situation.') . '</p>';
+  }
+}
webchick’s picture

Status: Reviewed & tested by the community » Needs work

No, that's just contextual help for the admin page. We need one for the admin/help path.

ParisLiakos’s picture

d'oh sorry. got it now

amateescu’s picture

Assigned: amateescu » Unassigned

Glad that someone picked this up :)

Crell’s picture

Issue tags: +Novice

Just adding a help hook is a valid Novice task if anyone wants to pick it up.

jacobbednarz’s picture

Status: Needs work » Needs review

ban.patch queued for re-testing.

andypost’s picture

Suppose we should commit current patch and proceed with #1570102: [Policy] Deprecate Ban module
Better hook_help text could be written in follow-ups or leave it for contrib

Cameron Tod’s picture

FileSize
1.16 KB
36.03 KB

Small cleanup to docs in hook_help() - I guess this needs a handbook page too?

ParisLiakos’s picture

+      $output .= '<dd>' . t('If you would like to configre IP addreses follow the <a href="@bans">Ban IP administration page</a>.', array('@bans' => url('admin/config/people/ban'))) . '</dd>';

Should be configure

Cameron Tod’s picture

Status: Needs review » Needs work

I think I attached an interdiff generated in the wrong direction, sorry. Here's the correct one.

See below for the hook_help in the patch in #46:

+++ b/core/modules/ban/ban.moduleundefined
@@ -0,0 +1,154 @@
+function ban_help($path, $arg) {
+  switch ($path) {
+    case 'admin/help#ban':
+      $output = '';
+      $output .= '<h3>' . t('About') . '</h3>';
+      $output .= '<p>' . t('The Ban module allows administrators to ban visits to their site from given IP addresses.') . '</p>';
+      $output .= '<h3>' . t('Uses') . '</h3>';
+      $output .= '<dl>';
+      $output .= '<dt>' . t('Banning IP addresses') . '</dt>';
+      $output .= '<dd>' . t('Administrators can enter IP addresses to ban on the <a href="@bans">IP address bans</a> page.', array('@bans' => url('admin/config/people/ban'))) . '</dd>';
+      $output .= '</dl>';
+      return $output;
+    case 'admin/config/people/ban':
+      return '<p>' . t('IP addresses listed here are banned from your site. Banned addresses are completely forbidden from accessing the site and instead see a brief message explaining the situation.') . '</p>';
+  }
+}
Cameron Tod’s picture

Status: Needs work » Needs review
ParisLiakos’s picture

Ah, wrong, interdiff, i see:)
I am tempted to rtbc this now, but i will leave it for someone else.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Now patch should pass a doc gate!

Suppose we just need a change-notice after this patch lands, also in case of success #1570102: [Policy] Deprecate Ban module docs page would live in contrib space

sun’s picture

Assigned: Unassigned » sun
Status: Reviewed & tested by the community » Needs review

Awesome!

+++ b/core/modules/ban/ban.admin.inc
@@ -0,0 +1,115 @@
+function ban_page($default_ip = '') {

Can we rename this to ban_admin_page()?

+++ b/core/modules/ban/ban.install
@@ -0,0 +1,37 @@
+function ban_schema() {
...
+  $schema['banned_ips'] = array(

When owned by ban module, the table name should adhere to our db table name standards, I think; i.e.:

1) 'ban' instead of 'banned'

2) The table stores records of individual IP addresses; each IP can be seen as a single "entity". Thus, the table name should be singular, not plural.

I.e.: 'ban_ip'

+++ b/core/modules/system/system.install
@@ -1951,6 +1928,42 @@ function system_update_8019() {
+function system_update_8020() {
...
+  if ($blocked_ips_exists || module_exists('statistics')) {

module_exists() generally shouldn't be used in module updates. Instead, we should query the {system} table and check whether .schema_version is >= 0, which means that the statistics module has been installed.

In general, however, I wonder why we're adding a hard dependency on Ban module to Statistics module. I think that should be a soft dependency; i.e., optional support for Ban module, if it is enabled. However, I'm fine with doing that decoupling in a follow-up issue.

+++ b/core/modules/system/system.install
@@ -1951,6 +1928,42 @@ function system_update_8019() {
+    module_enable(array('ban'));

This should use update_module_enable().

+++ b/core/modules/system/system.install
@@ -1951,6 +1928,42 @@ function system_update_8019() {
+    // Copy the blocked ips from the old {blocked_ips} table to the new
+    // {banned_ips} table.
+    $result = db_query("SELECT ip FROM {blocked_ips}");
+    $query = db_insert('banned_ips')->fields(array('ip'));
+    foreach ($result as $blocked_ip) {
+      $query->values(array(
+        'ip' => $blocked_ip->ip,
+      ));
+    }
+    $query->execute();
...
+  // Drop old table.
+  db_drop_table('blocked_ips');

Why don't we use db_rename_table() here?

+++ b/core/modules/system/system.install
@@ -1951,6 +1928,42 @@ function system_update_8019() {
+    // Remove legacy action.
+    db_delete('actions')
+      ->condition('aid', 'system_block_ip_action')
+      ->execute();

Why are we deleting an existing action instead of replacing it with the new one?

sun’s picture

FileSize
9.15 KB
35.89 KB

Renamed ban_page() into ban_admin_page().
Renamed {banned_ips} table into {ban_ip}.
Replaced module_exists() in system module update with schema_version check.
Fixed module_enable() must be update_module_enable().
Fixed system module update to rename the database table and action.

Cameron Tod’s picture

Status: Needs review » Needs work

Looks pretty great to me :)

One thing though:

+++ b/core/modules/statistics/statistics.admin.incundefined
@@ -157,8 +157,8 @@ function statistics_top_visitors() {
+    $ban_link = $account->iid ? l(t('unban IP address'), "admin/config/people/ban/delete/$account->iid", array('query' => $destination)) : l(t('ban IP address'), "admin/config/people/ban/$account->hostname", array('query' => $destination));
+    $rows[] = array($account->hits, ($account->uid ? theme('username', array('account' => $account)) : $account->hostname), format_interval(round($account->total / 1000)), (user_access('ban IP addresses') && !$account->uid) ? $ban_link : '');
   }

These two lines are pretty hard to read as written - do we want to block them out to multiple lines?

andypost’s picture

Why are we deleting an existing action instead of replacing it with the new one?

see #26

Old action could be removed with "remove orphan actions" functionality but new one are self-registered. try to rename and you got duplicate key error

sun’s picture

Status: Needs work » Needs review
FileSize
2.42 KB
36.05 KB

@cam8001: That's rather out of scope for this patch, but ok, cleaned it up.

@andypost: Nah.

Fixed ban IP action is not properly updated in system module update.
Cleaned up code garbage in statistics_top_visitors().

andypost’s picture

@sun Great, I learn a update_module_enable()

The only question I have here is should we keep a hard dependency on Ban for Statistics. Suppose this could be easily removed

sun’s picture

FileSize
3.98 KB
36.42 KB

Removed hard dependency on Ban module from Statistics module.

Lars Toomre’s picture

Reading through the patch, the following items should be addressed in a follow up issue to bring this new module up to D8 documentation/coding standards.

+++ b/core/modules/ban/ban.admin.incundefined
@@ -0,0 +1,115 @@
+ *
+ * @param $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.

Needs type hinting. Also should start with '(optional)' as well as specifying what the default value is.

+++ b/core/modules/ban/ban.admin.incundefined
@@ -0,0 +1,115 @@
+/**
+ * Defines the form for banning IP addresses.
+ *
+ * @see ban_ip_form_validate()
+ * @see ban_ip_form_submit()
+ *
+ * @ingroup forms

Missing '@param string $default_ip' directive...

+++ b/core/modules/ban/ban.admin.incundefined
@@ -0,0 +1,115 @@
+/**
+ * IP deletion confirm page.
+ *
+ * @see ban_ip_delete_submit()

Missing directive '@param array $iid'.

+++ b/core/modules/ban/ban.moduleundefined
@@ -0,0 +1,153 @@
+ *
+ * @param $ip
+ *   IP address to check.
+ *
+ * @return bool
+ *   TRUE if access is denied, FALSE if access is allowed.

Needs type hinting.

+++ b/core/modules/ban/ban.moduleundefined
@@ -0,0 +1,153 @@
+  $blocked_ips = variable_get('blocked_ips');
+  if (isset($blocked_ips) && is_array($blocked_ips)) {
+    $denied = in_array($ip, $blocked_ips);

Needs conversion to CMI sub-system.

+++ b/core/modules/ban/ban.moduleundefined
@@ -0,0 +1,153 @@
+  // Only check if database.inc is loaded already. If
+  // $conf['page_cache_without_database'] = TRUE; is set in settings.php,
+  // then the database won't be loaded here so the IPs in the database
+  // won't be denied. However the user asked explicitly not to use the
+  // database and also in this case it's quite likely that the user relies
+  // on higher performance solutions like a firewall.

Needs to be rewrapped to better use 80 characters per line.

+++ b/core/modules/ban/ban.moduleundefined
@@ -0,0 +1,153 @@
+ * @param $ip
+ *   IP address to check. Prints a message and exits if access is denied.

Needs type hinting.

+++ b/core/modules/ban/ban.moduleundefined
@@ -0,0 +1,153 @@
+    print 'Sorry, ' . check_plain(ip_address()) . ' has been banned.';

Do not understand why we are check_plain() the result from ip_address() instead of variable $ip.

+++ b/core/modules/ban/ban.moduleundefined
@@ -0,0 +1,153 @@
+ * @param $iid integer
+ *   The ID of the banned IP address to retrieve.
+ *
+ * @return
+ *   The banned IP address from the database as an array.

Need to correct @param directive and add array type hine to @return directive.

+++ b/core/modules/ban/lib/Drupal/ban/Tests/IpAddressBlockingTest.phpundefined
@@ -0,0 +1,88 @@
+    // Submit your own IP address. This fails, although it works when testing
+    // manually.
+    // TODO: on some systems this test fails due to a bug or inconsistency in cURL.
+    // $edit = array();
+    // $edit['ip'] = ip_address();
+    // $this->drupalPost('admin/config/people/ban', $edit, t('Save'));
+    // $this->assertText(t('You may not ban your own IP address.'));

This portion of the test needs to be uncommented and resolved.

+++ b/core/modules/statistics/statistics.admin.incundefined
@@ -157,8 +157,18 @@ function statistics_top_visitors() {
+    if ($account->iid) {
+      $ban_link = l(t('unban IP address'), "admin/config/people/ban/delete/$account->iid", array('query' => $destination));
+    }
+    else {
+      $ban_link = l(t('ban IP address'), "admin/config/people/ban/$account->hostname", array('query' => $destination));
+    }
+    $row = array();
+    $row[] = $account->hits;
+    $row[] = ($account->uid ? theme('username', array('account' => $account)) : $account->hostname);
+    $row[] = format_interval(round($account->total / 1000));
+    $row[] = (user_access('ban IP addresses') && !$account->uid ? $ban_link : '');
+    $rows[] = $row;

Based on comments in other issues, this type of clean-up of code should be done in a follow up issue. It makes the movement of code easier for all to review.

Lars Toomre’s picture

More immediately before this patch is applied, can we address:

+++ b/core/modules/ban/ban.installundefined
@@ -0,0 +1,35 @@
+    'fields' => array(
+       'iid' => array(

The indentation of 'iid' appears one character too many.

+++ b/core/modules/system/system.installundefined
@@ -1979,6 +1956,43 @@ function system_update_8019() {
+  $blocked_ips_exists = db_query_range('SELECT 1 FROM {blocked_ips}', 0, 1)->fetchField();
+  $statistics_installed = db_query_range('SELECT 1 FROM {system} WHERE type = :type AND name = :name AND schema_version >= 0', 0, 1, array(
+    ':type' => 'module',
+    ':name' => 'statistics',
+  ))->fetchField();
+  if ($blocked_ips_exists || $statistics_installed) {

I am not sure if this has been thought fully through. Shouldn't somewhere in this patch include some change to statistics module indicating a dependency on or function call from the new ban module to use the table that is now defined in ban module?

Lars Toomre’s picture

My comment #60 was written before I saw #58.

Lars Toomre’s picture

Regarding my second comment in #60, what is bothering me is 'Under what conditions prior to this new module, would the statistics module be installed but the blocked_ips table not exist?' I am unaware of one since that table prior to this was integral to statistics module. Hence, I think all we need is 'if ($locked_ips_exists)'. The OR about statistics module being installed is not needed.

sun’s picture

FileSize
8.7 KB
36.51 KB

Fixed phpDoc of ban_admin_page().
Fixed phpDoc of ban_ip_form().
Fixed phpDoc of ban_ip_form() validation and submission handlers.
Fixed phpDoc, variable and function names for unbanning an IP address (record).
Fixed phpDoc of ban_is_denied().
Fixed inappropriate and bogus changes to ban_is_denied().
Fixed phpDoc and comments of ban_block_denied().
Changed ban_block_denied() to use the passed in $ip address instead of calling ip_address() again.
Fixed phpDoc of ban_ip_load().
Fixed indentation of schema array and @file phpDoc blocks.
Removed condition for enabled Statistics module from System module update.

Two points have been intentionally left out:

  1. We will not convert to CMI in this patch. That would be 100% scope-creep.
  2. We will also not adjust or fix the IpAddressBlockingTest, with regard to the commented out lines in that test - they are not introduced here.
Lars Toomre’s picture

Overall, the patch in #63 looks quite good and brings the proposed new Ban module very close to D8 standards. Here are a few notes I made while reviewing the most recent patch. Thanks for your work in bringing this up to snuff @sun.

+++ b/core/modules/ban/ban.admin.incundefined
@@ -0,0 +1,126 @@
+/**
+ * Page callback; displays banned IP addresses.

Small nit... According to documentation standards, this should be '* Page callback: Displays'

+++ b/core/modules/ban/ban.admin.incundefined
@@ -0,0 +1,126 @@
+ * @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.

Needs a @see comment referencing where this callback was registered.

+++ b/core/modules/ban/ban.admin.incundefined
@@ -0,0 +1,126 @@
+/**
+ * Form validation handler for ban_ip_form().

My understanding is that validate functions need @see to their submit counterpart and the opposite in submit functions. Both @see references appear to be missing here.

+++ b/core/modules/ban/ban.moduleundefined
@@ -0,0 +1,155 @@
+ * @file
+ * Enables banning of IP addresses.

Perhaps 'Provides capability to ban individual IP addresses.'?

+++ b/core/modules/ban/lib/Drupal/ban/Tests/IpAddressBlockingTest.phpundefined
@@ -0,0 +1,88 @@
+  /**
+   * Implement getInfo().

Not sure whether this phpblock is needed or not...

tstoeckler’s picture

+++ b/core/modules/ban/lib/Drupal/ban/Tests/IpAddressBlockingTest.phpundefined
@@ -0,0 +1,88 @@
+  /**
+   * Implement getInfo().

Not sure whether this phpblock is needed or not...

Weirdly our standard is not to have PHPDoc for getInfo() and setUp() in tests...

Lars Toomre’s picture

Well, if #65 is indeed the case, that means the docblock needs to be removed.

sun’s picture

FileSize
1.47 KB
36.54 KB

Final phpDoc adjustments.

@Lars Toomre:
I really appreciate the desire for quality [Been there, done that. ;)], but on a personal note, I'd recommend to spend some thoughts on balancing overly strict coding standards/style compliance with getting major things done. :)

Getting a feeling for the right balance certainly takes some time. It pretty much boils down to pragmatism and deciphering what can be done in a less major follow-up issue and what cannot — that is, all relative to the complexity of a particular change proposal at hand.

Perfectionism is the perfect enemy of progress. Especially when it comes to major change proposals, the primary focus for reviewers should actually be on the major [architectural] change proposal itself. Minor things can be happily hashed out in a separate follow-up patch or issue, which can already be created ahead of time, postponed on the major change. On top, such issues are an excellent opportunity for novice contributors to step into contributing to Drupal core. :)

Contrary to minor things, it makes sense to bear in mind that even the major change proposals are done and backed by volunteers, people like you and me, but who potentially spent an awful lot of time to figure out how to get the entire system to work with the new code. For them, it can be incredibly demotivating to see their major change proposals getting blocked on nitpicky reviews instead of moving those super-trivial issues out of the way into a follow-up issue.

So when balancing review issues, it is important to understand that we love to see bold proposals for Drupal core, and contributing to Drupal core ought to be fun, motivating, enjoyable, and respecting the work that others have put into a certain proposal or topic. It really matters to put things into relation.

I hope this helps, and I'm happy to discuss and clarify this some more in IRC or other means, if desired. (Let's not hi-jack this issue, please ;))

Intentionally omitted:

Needs a @see comment referencing where this callback was registered.

That's wrong and I don't understand how that can be in the handbook already. There was no agreement on that yet. In fact, API module was improved instead to discover function references in strings. Which in turn invalidates and eliminates the entire idea of adding manual @see references for (menu/router system) callbacks. This needs to be removed from the handbook page, and I'm fairly sure that the original issue that proposed it is still open (or re-opened). Anyway, irrelevant for this patch here.

sun’s picture

Sorry, forgot to mention: Created a new issue to prevent us from getting off-topic here:
#1791872: [Policy, no patch] Add special tag to identify issues ready for a high level only review

Lars Toomre’s picture

Thanks @sun. I appreciate that you have incorporated my suggested changes.

I also appreciate that contributing should be fun; my concern is that what is submitted should at least meet (what I understand from your own comments in the last two years) minimal coding/documentation standards. What was originally proposed did not, even though it was being copied from elsewhere in core.

Based upon your own and @catch comments in #1008166: Actions should be a module, I had no expectation that my comments in #59 would be incorporated here (look at my lead comment there). I expected this to be addressed in a follow-up issue. However, since you elected to make some changes in #63, it appeared that conforming to documentation standards in this case was appropriate.

If I was wrong to think so, please let me know.

sun’s picture

Yeah, I incorporated most of the review issues here, since 1) the amount of code being touched/moved is limited, 2) I understand the good intentions, and 3) I'm insane and can deal with it. ;)

In terms of complexity, this patch/issue is vastly different to #1008166: Actions should be a module, even though they might appear to aim for a seemingly similar goal. The are multiple differences, but the most prominent is that this patch impacts the early bootstrap phase of Drupal core (on every request).

That's an essential architectural difference, and although I am confident that the change proposal is correct, it is that architectural difference that actually needs attention. :)

That said, I consider this patch RTBC.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

I think this ready to be decoupled. A follow-ups are reqired after commit:
1) use state (k/v) for cache of small banlist
2) find a place in new kernel or bootstrap

+++ b/core/modules/ban/ban.module
@@ -0,0 +1,155 @@
+  $blocked_ips = variable_get('blocked_ips');
...
+  // If $conf['page_cache_without_database'] = TRUE; is set in settings.php,
+  // then the database is not available yet, so IPs recorded in the database
+  // won't be denied. However, the user asked explicitly not to use the
+  // database, and in this case it's also quite likely that the user relies
+  // on higher performance solutions like a firewall.
+  elseif (class_exists('Drupal\Core\Database\Database', FALSE) && function_exists('db_query')) {
+    $denied = (bool) db_query("SELECT 1 FROM {ban_ip} WHERE ip = :ip", array(':ip' => $ip))->fetchField();
+  }

This require follow-up issue to Fix variable_get('blocked_ips') and maybe to fix page cache state...
I think there's no need to get page from cache, so this module mostly should work at the end of previous boostrap phase or right after config init?

DRUPAL_BOOTSTRAP_CONFIGURATION => (Ban? return 404 response and fast 404) -> DRUPAL_BOOTSTRAP_PAGE_CACHE=> (Ban? and return 404 + some theming)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Good clean up. Thanks!

andypost’s picture

Title: Move IP blocking into its own module » Change notification for: Move IP blocking into its own module
Priority: Normal » Major
Status: Fixed » Active

We needa change notification and follow up discussion in #1570102: [Policy] Deprecate Ban module
The suggested follow-up to add ip-ranges support for module

catch’s picture

Priority: Major » Critical
ParisLiakos’s picture

Issue tags: +Needs change record

adding tag

ParisLiakos’s picture

Status: Active » Fixed

And here is a change notice:
http://drupal.org/node/1794068

Feel free to improve it, thats my first one :p

podarok’s picture

Title: Change notification for: Move IP blocking into its own module » Move IP blocking into its own module
Assigned: sun » Unassigned
Status: Fixed » Needs review
Issue tags: -Needs change record

#76 looks good, removing tag, Back to right title
and due to #46 #47 waiting for Needs Documentation review

catch’s picture

Priority: Critical » Normal
Status: Needs review » Fixed

#46/#47 were included in the patch that was committed I think, should be fine marking this fixed.

sun’s picture

Component: base system » ban.module
Status: Fixed » Needs review
FileSize
2.09 KB

We missed and overlooked a couple of things.

None of this can be tested, since the test runner would lock itself out of the system, AFAICS.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

+1, sad that we still have no actual tests for actions (no triggers)

Lars Toomre’s picture

I created a new issue #1794754: Convert ban_boot() to an event listener for other changes after #79 is committed and will be posting an initial patch there shortly (with some small documentation changes). The chief issue that we must follow up on is the refactoring of variable_get() to config sub-system.

Lars Toomre’s picture

Apparently #1479466: Convert IP address blocking to config system has been open for several months about moving IP blocking list to the config system. I did not realize this when I opened #1794754: Convert ban_boot() to an event listener as follow-up to this issue. That issue will include other updates about how new Ban module might fit into new routing scheme. Thanks @sun for your information.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed #79.

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.