🇦🇺Australia @darvanen

Thanks @leslieg :)

@dieterholvoet the context is in comment #13 - however - @hikkypo if you're sure enough that you're right you should edit the MR like @dieterholvoet said rather than providing a patch to the MR, that's not a normal procedure in the Drupal space.

Regardless of those issues, I think that section of code is too dense to be grokked clearly by an incoming developer. My opinion is that the various tests in that series should be separated named as variables before being combined the way they are now.

Well... I'm a little ashamed to admit it but it seems AI found the solution I could not.

That said, I had to push it *very* hard against itself with the tests.

I'm going to come back later to read all of these changes again and decide if they're actually worth comitting. I welcome any feedback in the interim.

So... exposing debug mode config to the frontend is easy, I've pushed that.

Unfortunately logging to watchdog on the backend is currently up to any implementing code as we're not actually making any calls using the client in this module yet.

Calling code will need to inspect the response for debug messages and log them.

I'll keep it in mind if we get any action in ✨ Integrate with ECA? Active or similar.

- Added JS test for frontend project token
- Needed to include the core/drupalSettings library in case it is not included anywhere else
- Tidied up the module file a bit to better meet coding standards

Looks like that was a false positive

I think you have some serious issues with your infrastructure if you need js files to be executable for them to load in the browser.

The collapsiblock.js the browser is trying to load is already readable by all groups and users, that should be enough.

I'm going to mark this postponed in case there's more to it but after a while I will close it.

Oh that makes way more sense, lol.

Hi @bobi-mel,

After working with you on ✨ New graphic for open and close Active I'm happy to give you restricted co-maintainership of this module as I can see the effort you are putting in but I think some guard rails are still warranted.

If you still want to be involved I can give you access to write to VCS and maintain issues. This will give you the ability to commit changes to the main repo, however I will reserve "Administer Releases" for now as I want to check before we release.

Sorry it took me a few months to get to this, life is pretty hectic.

I've looked it over, fixed a few things where merge conflicts weren't resolved correctly or needed a first-language English speaker (no judgement).

I'm merging this now.

I can see how this is undesirable, the active class action is only meant for menus, not pagers.

Why you would want to collapse a pager is beyond me but sure, let's put this in.

darvanen → made their first commit to this issue’s fork.

Since we're making a hook we'll need to make a token_filter.api.php file as well as the documentation page.

Postponing on the parent issue, thank you for finding that. Not closing this until that one is sorted in case there are follow up actions for this module.

Fixing credit attribution

Actually you know what, I agree. Done.

Thanks for this, and I appreciate the follow-up for phpcs but that rule has been reversed so I've rolled back that change.

darvanen → made their first commit to this issue’s fork.

Thanks for the attempts @sayan_k_dutta and @koustav_mondal. I'm crediting you for the effort because I believe they were genuine attempts (please note this is not normally how credits work, I'm just feeling generous today).

Take a look at how I did fix it next time you have a few moments to spare.

I'm in favour of using Drupal's API by default having run into reverse proxy issues in the past.

Bumping this issue for discussion.

If approved, this might be grounds for a change in major version.

As Drupal 7 is no longer supported I am going to close this as outdated.

Please feel free to re-open if you encounter this issue in the currently supported branch(es).

This looks very much like a problem with the infrastructure in question rather than the module - which is backed up by there being no activity here for 7 years.

I'm going to close this as outdated, but feel free to reopen if you discover there's an actual issue with the module here.

After 7 years with no activity I think it's safe to say this feature request is not going anywhere.

Personally I think the password reset system is suitably protected by the requirement to use a link sent to the users registered email address.

Release to come when ✨ Not IPv6 compatible Needs work goes in.

Seems Symfony's IPUtils doesn't check for valid CIDR ranges so that had to be put back in for IPv4 addresses.

I will admit I asked Junie (AI) to add some IPv6 testing data to the Unit test but I have looked them over and they seem right to me.

All tests are passing both locally and in the CI.

I'll leave this a short while for review and if there are no comments I'll commit it.

This change is badly needed for the test suite to run green because gitlab CI uses IPv6 in the browser, however, it is currently failing at the Unit test:

There were 3 failures:
1) Drupal\Tests\restrict_by_ip\Unit\UnitTest::testIpFailValidation with data set #3 ('127.0.0.1/8', 'Invalid /8')
Failed asserting that exception of type "Drupal\restrict_by_ip\Exception\InvalidIPException" is thrown.
/builds/project/restrict_by_ip/vendor/bin/phpunit:122
2) Drupal\Tests\restrict_by_ip\Unit\UnitTest::testIpFailValidation with data set #4 ('127.0.0.1/16', 'Invalid /16')
Failed asserting that exception of type "Drupal\restrict_by_ip\Exception\InvalidIPException" is thrown.
/builds/project/restrict_by_ip/vendor/bin/phpunit:122
3) Drupal\Tests\restrict_by_ip\Unit\UnitTest::testIpFailValidation with data set #5 ('127.0.0.1/24', 'Invalid /24')
Failed asserting that exception of type "Drupal\restrict_by_ip\Exception\InvalidIPException" is thrown.
/builds/project/restrict_by_ip/vendor/bin/phpunit:122

I think 8 years with nobody else chiming in to request this feature means we can park it. Please feel free to re-open against the current branch if you wish to contribute to this feature.

I also think the request here is *way* beyond the current scope of the module.

As Drupal 7 is no longer supported I'm moving this to fixed.

I've rolled back the changes I made as they didn't help.

I've also rebased on the latest work on 8.x-1.x which I should have done in the beginning.

@rocketeerbkw's changes for beta2 solved the redirect issue which only leaves some UI tests. I'm going to commit this and move to managing follow-ups.

The UITests are throwing ipv6 related errors on gitlab, we already have a follow-up for that: ✨ Not IPv6 compatible Needs work

@rocketeerbkw thanks for the follow-up

I Just wanted to mention here that the tests are working correctly outside of this issue and gitlab CI. I haven't reviewed/tested this issue.

This is what I took your comment to mean, I really appreciate knowing that they're functioning correctly - my only concern is what may have changed in Drupal 11 that might have caused failures.

On that note I've tracked down the failure of the redirect test to the redirect function failing when the destination parameter is set to an invalid path - 'node' doesn't exist on minimal install in Drupal 11.

I confirmed the same behaviour on Drupal 10 (with a different destination param) so I've pushed a tiny change to use '/' instead on those two test and will open a follow-up issue to fix the existing bug.

Tested locally on Drupal 11:

I can confirm the UI tests are passing locally, happy to let those be a follow-up. However it looks like the destination parameter is overriding the redirect to the access denied page which is something the tests seem to be there to ensure doesn't happen. Going to dig a little deeper into that.

All other tests are running fine so I don't think we need to mess with IPs and requests.

Thanks very much @rocketeerbkw that's really good to know - may I ask which version of core you ran them against?

So we do, thanks. I'll just add the necessary fields to it.

darvanen → created an issue.

@nicrodgers if I was more familiar with the source code I might agree with you but it is a long time since I interacted with or even used this module. I put my hand up to maintain years ago and it just resolved - not a complaint, just explanation. I intend to take my custodial duties seriously.

The tests are failing which either means:

• the tests are not D11 compatible, or
• the ecosystem has changed enough that they're legitimately failing

It's good to know that you have been using this patch for a long time (and presumably so have others) but I'm not willing to cut a release based on that alone. I want people picking up the module or returning to it to have the most possible confidence in the new release.

I will be looking at the tests when I can manage to do so, I'm not putting this off awaiting someone else to fix them - though help is welcome :)

After a chat with @nterbogt I've added proper resolvers for the citation data rather than letting it happen magically.

@nterbogt made the case elsewhere for pinning the major version instead which I now agree with, and have done.

darvanen → changed the visibility of the branch 3523508-upgrade-to-v1.5 to hidden.

Still a valid request in Drupal 8+ but will need work.

Agree this is vaulable, just needs tests to make it part of the module.

I'm sorry, I don't see what renaming the config property of these classes could possibly have to do with a circular reference to a `recript_by_ip.current_user` service .

I'm postponing this issue pending further explanation and may close later if there is no activity.

[edit: I see that service is actually defined by this module, I missed it the first time around - however the remainder of this comment stands]

Agree this looks good, just needs tests to cement it.

If someone is interested in creating said tests, you may find it easier to wait for 📌 Drupal 11 compatibility Active to get in or help with that, as that issue will activate gitlab CI.

As this ticket has had no activity for many years, is for 7.x and Drupal 7 is now out of support I am closing this ticket as outdated.

As this ticket is for 7.x and Drupal 7 is now out of support I am closing this ticket as outdated.

Please feel free to open a new issue regarding i18n for the current version of the module if required.

As this ticket is for 7.x and Drupal 7 is now out of support I am closing this ticket as outdated.

I think the practice of overriding $config in the settings file is well-known by now.

Echoing #3.

Also:

+++ b/src/IPTools.php
@@ -24,11 +33,13 @@ class IPTools implements IPToolsInterface {
+      $ip_address = $this->requestStack->getCurrentRequest()->getClientIp();

This kind of magic fallback might bring some users unstuck.
If we're going to provide this option it needs to have an explicit opt-in on the settings page please, and tests.

What is being requested here is a combination of functionality that is beyond the scope of this module in my opinion.

Happy to discuss if someone has these needs in the current version. Closing as outdated.

Unfortunately merging this without resolving the phpunit tests will result in an error state on the module page. I would like the phpunit errors resolved first, and as it probably involves some level of compatibility issues let's do it as part of 📌 Drupal 11 compatibility Active where I see this change has also already been made.

Really appreciate the effort you have been putting into this module @nikolay shapovalov

There has been a Drupal 8 compatible release for this module for some time. Thanks for the report but I'm going to close this now.

I'm going to leave this open for the bot to create patches in, but if you come here looking to contribute to module please go to the other issue for 📌 Drupal 11 compatibility Active

@nicrodgers hi, new maintainer here. I agree with your assessment on the comments on the MR - I am concerned about the failing phpunit tests though, particularly as this is a module people will be using for security purposes. We need to resolve those before merging any of this.

It may be that the tests are out of date in some fashion but that needs to be thoroughly researched.

No stress about the linting errors, those can be a follow-up.

I have also added a new citationData field. A query might look like

  query Query($query: String!) {
    vertexAiSearchQuery(query: $query) {
      summary(withCitationsInText: true),
      citationData {
        startIndex
        endIndex
        referenceIndexes
      }
    }
  }

The citation data from the Vertex API has the capability to allocate more than one reference index to a citation so this is reflected in the query results:

I have added a parameter to the summary field:

    summary(
        withCitationsInText: Boolean
    ): String

With that set to true your summary text will have the citations embedded in brackets like

The Driving Test is a practical on-road test that focuses on your ability to perceive hazards and respond to them [2]. A Testing Officer assesses your driving skills, decision making, your awareness of other road users and how you share the road [1]. You will be notified of the result after the test [3]. The Driver Knowledge Test is the first step in getting your driver licence [2]. To sit the driving test and apply for your P1 licence, you'll need to pass the Hazard Perception Test first [2]. For heavy vehicle drivers, the test must be done in a vehicle appropriate to your licence type [3]. For a class MC licence, the driving test must be done in a HC vehicle type [3].

The numbers refer to the top search results in numeric order.

darvanen → created an issue.

I pinned it to the minor version ~1.5.0

darvanen → created an issue.

Code looks good - I'm afraid I don't have access to the projects I was using this on any more so I can't try it out.

@apaderno yes, happy to become caretaker and carry out the tasks I outlined earlier, thanks.

Fixed the phpcs failure but this definitely needs work on tests, don't have time for that at the moment.

Oh I didn't realise, I should have checked the home page, thanks for that, I've rebased.

darvanen → changed the visibility of the branch 2958658-permission-reorder-terms to active.

darvanen → changed the visibility of the branch 2958658-permission-reorder-terms to hidden.

#9 did fix the issue BUT I don't want to give reset permission to most users, it will cause utter havoc on our system.

#15 passes manual testing, thank you.

As there are no pipelines running any more I've put this back to NR.

Closing as a duplicate of 🐛 Taxonomy Term List page is broken when users don't have permission to edit/create terms Active

That approach works for me 😊

This is very interesting @mparker17, I see there hasn't been much discussion and I wonder if you feel like that is blocking progress?

We use this module with a very large dataset, I'd be happy to try out any protoypes.

fubarhouse → credited darvanen → .

I would love pass2 to inherit the attributes of pass1 with an override capacity… but I suspect that will break a great many sites.
I’m not sure getting the pattern right in this instance is worth the pain we’re likely to cause for a small heap of intermediate site builders.

Hi @sirclickalot,

This module is for enabling tokens in filtered text fields. It does not have any effect on views rewriting, they do not use the filters you find at /admin/config/content/formats.

I had a quick look and didn't see any modules which do affect that part of views but there are plenty which are more adjacent than this one.

I'm sorry, you'll need to find something more specific to your needs, or perhaps even write it.

Ah, got it.

Doing this to the test form does demonstrate the problem. wasn't sure if I should push the change though

Index: core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.php b/core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.php
--- a/core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.php	(revision 62764410c50313e7fd0068cde2f5f78e8c846d23)
+++ b/core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.php	(date 1740698679180)
@@ -773,7 +773,9 @@
     ];
     // Ajax trigger.
     $form['ajax_reload'] = [
-      '#type' => 'checkbox',
+      '#type' => 'select',
+      '#options' => [0 => 0, 1 => 1],
+      '#default_value' => 0,
       '#title' => 'Check me to reload elements with the states_ajax_test_wrapper',
       '#ajax' => [
         'callback' => '::buildAjax',
@@ -818,6 +820,7 @@
         '#states' => [
           'enabled' => [
             ':input[name="checkbox_trigger"]' => ['checked' => TRUE],
+            ':input[name="ajax_reload"]' => ['value' => 1],
           ],
         ],
       ];

Yes that means there is a dependent/dependee pair that share both ajax and states relationships. It comes into play (at least for us) when ajax is used to update large sections of form that can have portions which use state to manage visibility based on the ajax trigger. And the form in question *did* work in 10.2

I was unable to get the test form to repeat the behaviour I'm seeing - possibly because it uses a checkbox as the ajax trigger rather than a select element, however I did discover the cause of our issue. The trigger events needed to be namespaced so that other events weren't stripped off along with them.

I have pushed a very small amendment to that effect.

Commenting out line 624 of the adjusted states.js stops the ajax event from being removed.

I applied the changes in MR9300 to a site running 10.4.3 and now an element that has both ajax and states dependees only runs its ajax handler once. I'm going to keep digging but if that sparks any ideas please let me know.

darvanen → created an issue.

An alternative (for maintainers only) if you don't want to mark an issue as fixed because it was solved elsewhere - is to use the username search box in the crediting section of the solved issue to credit users from the first issue.

Marking it fixed is easier though :)

The button label doesn't get overridden *there*, but it does get overridden in \Drupal\webform\Element\WebformActions::processWebformActions

      // Apply custom label.
      $has_custom_label = !empty($element[$button_name]['#webform_actions_button_custom']);
      if (!empty($element['#' . $settings_name . '__label']) && !$has_custom_label) {
        if (isset($element[$button_name]['#type']) && ($element[$button_name]['#type'] === 'link')) {
          $element[$button_name]['#title'] = $element['#' . $settings_name . '__label'];
        }
        else {
          $element[$button_name]['#value'] = $element['#' . $settings_name . '__label'];
        }
      }

@zoeblair is this an actual problem with the module? If so can you provide some steps to reproduce?

I think this goes beyond the remit of the module, sorry @ressa.

There are just so many different ways one might want to alter what happens when elements are clicked.

I really think you're getting into business-logic territory - i.e. that specific use-case would not be wanted by more than a tiny percentage of user of this module. I recommend you do that as part of your own theme where desired.

Feel free to continue the discussion despite the status change, or you can ping me on Slack, the #contribute channel is probably suitable.

Looking pretty good @bobi-mel, please update the schema and settings keys and labels to be more like the updated form.

Additional commits look good to me and the proof is in the green tests. However standard review procedures prevent me from marking this RTBC. Thanks for the update @keszthelyi.

Good start @vinayakmk47, thanks. Unfortunately the Drupal docs system doesn't support markup though.