- Status changed to Needs review
over 1 year ago 6:44pm 23 January 2023 - 🇮🇳India ankithashetty Karnataka, India
Fixed the phpunit test errors in #51, thanks!
- Status changed to Needs work
over 1 year ago 8:01pm 23 January 2023 - 🇺🇸United States smustgrave
function ban_update_8700() {
This needs to be updated for D10. - 🇮🇳India ankithashetty Karnataka, India
Thanks for the review @smustgrave!🙌🏼
Updated the patch as requested.
Thanks! - Status changed to RTBC
over 1 year ago 3:40pm 24 January 2023 - 🇺🇸United States smustgrave
Thanks for the quick turnaround
On Drupal 10.1. with a standard install
Tested the update hook does pass.
Uploading a screenshot of what the UI now looks like (added IS template while I was at it)
Running the tests without the fix I getDrupal\Core\Database\DatabaseExceptionWrapper : SQLSTATE[HY000]: General error: 1 no such column: bip.ip_range_end: SELECT "bip"."iid" AS "iid" FROM "test70272775"."ban_ip" "bip" WHERE ("bip"."ip" = :db_condition_placeholder_0) AND ("bip"."ip_range_end" = :db_condition_placeholder_1); Array ( [:db_condition_placeholder_0] => 16909316 [:db_condition_placeholder_1] => 16909319 )
Changes look good
- Status changed to Needs work
over 1 year ago 4:06pm 24 January 2023 - 🇬🇧United Kingdom longwave UK
As well as the below comments I also wonder that if instead of arbitrary start and end IPs, we should support CIDR ranges, e.g. 192.168.0.0/16 to ban all of 192.168.0.0 to 192.168.255.255. That would be easier to store and parse.
-
+++ b/core/modules/ban/src/BanIpManager.php @@ -29,41 +29,138 @@ public function __construct(Connection $connection) { + $group_ip = $query->andConditionGroup() + ->condition('bip.ip', $ip_long, '<=') + ->condition('bip.ip_range_end', $ip_long, '>=');
Wondering if this works as intended in all cases, given that the
ip
andip_range_end
columns are defined as strings, but here we seem to be doing a numeric comparison. -
+++ b/core/modules/ban/src/BanIpManagerInterface.php @@ -12,11 +12,13 @@ interface BanIpManagerInterface { + * @param array $options + * Options array.
What are the possible options? We need to document this.
-
+++ b/core/modules/ban/src/BanIpManagerInterface.php @@ -31,16 +33,20 @@ public function findAll(); + * The end of the IP address to ban (optional).
ban -> unban
Also, what happens with overlapping ranges?
-
+++ b/core/modules/ban/src/Form/BanAdmin.php @@ -108,14 +116,55 @@ public function buildForm(array $form, FormStateInterface $form_state, $default_ + if (!$ip_long) { + $form_state->setErrorByName('ip', $this->t('Only IPv4 is available for IP range.')); + } + if (!$ip_range_end_long) { + $form_state->setErrorByName('ip_range_end', $this->t('Only IPv4 is available for IP range.')); + }
None of the UI changes in this patch (such as these lines above) have test coverage.
-