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(-)
Comment | File | Size | Author |
---|---|---|---|
#79 | drupal8.ban-fixup.79.patch | 2.09 KB | sun |
#67 | platform.ban_.65.patch | 36.54 KB | sun |
#67 | interdiff.txt | 1.47 KB | sun |
#63 | platform.ban_.63.patch | 36.51 KB | sun |
#63 | interdiff.txt | 8.7 KB | sun |
Comments
Comment #1
catchActually 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.
Comment #2
Crell CreditAttribution: Crell commentedI 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.
Comment #3
catchPatch was missing hunk from system.install
Comment #4
sunComment #7
catchStatistics 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.
Comment #9
podaroksubscribe
Comment #10
catch#7: ban.patch queued for re-testing.
Comment #11
joachim CreditAttribution: joachim commented> Statistics module needs to depend on ban module.
That's going to look like a WTF on D8 :/
Could you not just wrap these bits up in a module_exists() ?
Powered by Dreditor.
Comment #12
catch@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.
Comment #13
mirocow CreditAttribution: mirocow commentedsubscribe
Comment #14
aspilicious CreditAttribution: aspilicious commentedGood time for a doc cleanup. Still needs the upgrade path :)
Comment #15
marcingy CreditAttribution: marcingy commentedInitial attempt at upgrade path.
Comment #16
marcingy CreditAttribution: marcingy commentedPutting 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.
Comment #17
marcingy CreditAttribution: marcingy commentedNew version which renames exist trigger assignments for blocking ips and then cleans up the old action from the actions table.
Comment #18
marcingy CreditAttribution: marcingy commentedWe 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.
Comment #19
andypost#17: 1161486-ban-module-doc-cleanup-17.patch queued for re-testing.
Comment #21
amateescu CreditAttribution: amateescu commentedI'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".
Comment #22
andyposthere'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
Comment #23
andypost- fixed @file block of test
- changed assert messages do not use t()
Comment #24
RobLoachOh, I like this a lot. It looks pretty close to RTBC as well!
The "blocked_ips" schema should probably be removed from system.install since we're moving it over to ban module.
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.
Comment #25
Crell CreditAttribution: Crell commentedandypost 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.
Comment #26
andypost@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
Comment #27
ParisLiakos CreditAttribution: ParisLiakos commentedAdded 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']
formsystem.install
per #24Comment #29
ParisLiakos CreditAttribution: ParisLiakos commentedYeah forgot removing
core/modules/system/lib/Drupal/system/Tests/System/IpAddressBlockingTest.php
when applying manually, sorry for thisComment #31
ParisLiakos CreditAttribution: ParisLiakos commentedBan 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
Comment #32
andypostFix 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
Comment #33
ParisLiakos CreditAttribution: ParisLiakos commentedMerged in andypost's changes to keep things less confusing and attached interdiffs
Comment #34
andypostnot needed
Change to $admin_user
Wrap 'manually.' to another line
Comment #35
ParisLiakos CreditAttribution: ParisLiakos commentedThanks, changed as above
Comment #37
andypostNow it's good to go. Manually tested patch and bot was wrong
Comment #38
catchThis 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.
Comment #39
ParisLiakos CreditAttribution: ParisLiakos commentedhmm it is there already:S
Comment #40
webchickNo, that's just contextual help for the admin page. We need one for the admin/help path.
Comment #41
ParisLiakos CreditAttribution: ParisLiakos commentedd'oh sorry. got it now
Comment #42
amateescu CreditAttribution: amateescu commentedGlad that someone picked this up :)
Comment #43
Crell CreditAttribution: Crell commentedJust adding a help hook is a valid Novice task if anyone wants to pick it up.
Comment #44
jacobbednarz CreditAttribution: jacobbednarz commentedban.patch queued for re-testing.
Comment #45
andypostSuppose 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
Comment #46
Cameron Tod CreditAttribution: Cameron Tod commentedSmall cleanup to docs in hook_help() - I guess this needs a handbook page too?
Comment #47
ParisLiakos CreditAttribution: ParisLiakos commentedShould be
configure
Comment #48
Cameron Tod CreditAttribution: Cameron Tod commentedI 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:
Comment #49
Cameron Tod CreditAttribution: Cameron Tod commentedComment #50
ParisLiakos CreditAttribution: ParisLiakos commentedAh, wrong, interdiff, i see:)
I am tempted to rtbc this now, but i will leave it for someone else.
Comment #51
andypostNow 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
Comment #52
sunAwesome!
Can we rename this to ban_admin_page()?
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'
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.
This should use update_module_enable().
Why don't we use db_rename_table() here?
Why are we deleting an existing action instead of replacing it with the new one?
Comment #53
sunRenamed 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.
Comment #54
Cameron Tod CreditAttribution: Cameron Tod commentedLooks pretty great to me :)
One thing though:
These two lines are pretty hard to read as written - do we want to block them out to multiple lines?
Comment #55
andypostsee #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
Comment #56
sun@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().
Comment #57
andypost@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
Comment #58
sunRemoved hard dependency on Ban module from Statistics module.
Comment #59
Lars Toomre CreditAttribution: Lars Toomre commentedReading 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.
Needs type hinting. Also should start with '(optional)' as well as specifying what the default value is.
Missing '@param string $default_ip' directive...
Missing directive '@param array $iid'.
Needs type hinting.
Needs conversion to CMI sub-system.
Needs to be rewrapped to better use 80 characters per line.
Needs type hinting.
Do not understand why we are check_plain() the result from ip_address() instead of variable $ip.
Need to correct @param directive and add array type hine to @return directive.
This portion of the test needs to be uncommented and resolved.
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.
Comment #60
Lars Toomre CreditAttribution: Lars Toomre commentedMore immediately before this patch is applied, can we address:
The indentation of 'iid' appears one character too many.
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?
Comment #61
Lars Toomre CreditAttribution: Lars Toomre commentedMy comment #60 was written before I saw #58.
Comment #62
Lars Toomre CreditAttribution: Lars Toomre commentedRegarding 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.
Comment #63
sunFixed 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:
Comment #64
Lars Toomre CreditAttribution: Lars Toomre commentedOverall, 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.
Small nit... According to documentation standards, this should be '* Page callback: Displays'
Needs a @see comment referencing where this callback was registered.
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.
Perhaps 'Provides capability to ban individual IP addresses.'?
Not sure whether this phpblock is needed or not...
Comment #65
tstoecklerWeirdly our standard is not to have PHPDoc for getInfo() and setUp() in tests...
Comment #66
Lars Toomre CreditAttribution: Lars Toomre commentedWell, if #65 is indeed the case, that means the docblock needs to be removed.
Comment #67
sunFinal 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:
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.
Comment #68
sunSorry, 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
Comment #69
Lars Toomre CreditAttribution: Lars Toomre commentedThanks @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.
Comment #70
sunYeah, 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.
Comment #71
andypostI 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
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)
Comment #72
Dries CreditAttribution: Dries commentedCommitted to 8.x. Good clean up. Thanks!
Comment #73
andypostWe needa change notification and follow up discussion in #1570102: [Policy] Deprecate Ban module
The suggested follow-up to add ip-ranges support for module
Comment #74
catchComment #75
ParisLiakos CreditAttribution: ParisLiakos commentedadding tag
Comment #76
ParisLiakos CreditAttribution: ParisLiakos commentedAnd here is a change notice:
http://drupal.org/node/1794068
Feel free to improve it, thats my first one :p
Comment #77
podarok#76 looks good, removing tag, Back to right title
and due to #46 #47 waiting for Needs Documentation review
Comment #78
catch#46/#47 were included in the patch that was committed I think, should be fine marking this fixed.
Comment #79
sunWe 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.
Comment #80
andypost+1, sad that we still have no actual tests for actions (no triggers)
Comment #81
Lars Toomre CreditAttribution: Lars Toomre commentedI 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.
Comment #82
Lars Toomre CreditAttribution: Lars Toomre commentedApparently #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.
Comment #83
catchCommitted/pushed #79.
Comment #84.0
(not verified) CreditAttribution: commentedUpdated issue summary.