🇦🇺Australia @darvanen

That's not a collapsiblock dependency issue, see the readme for js-cookie:
https://git.drupalcode.org/project/js_cookie/-/blob/1.x/README.md

:)

@larowlan keeps nudging me about the token system but I just don't think I have the capability for that right now. I'm starting to get my head around how TypedData fits into it but honestly I already see @berdir as the pseudo maintainer of that subsystem because of the token module and his extensive coverage of that.

I've been looking at menu_ui because I have some ideas for improvements once it starts getting big. Maintaining it for while might help those ideas solidify.

I've been using and contributing to Drupal since about 2010 and have a few modules I look after, the biggest of which (by install count) is token_filter. I'm a tech lead/solution architect in my day job and I attend local conferences (Drupal South), I'm reasonably well known and (I hope) approachable.

Name: Max Pogonowski
d.org username: darvanen

I can see how that would be useful, sure, happy to review work on this.

Would it be alright to add content_moderation to \Drupal\Tests\entity_hierarchy\Functional\ReorderChildrenWithRevisionsFunctionalTest and add some draft checking to the testReordering method?

No tests yet, but could use a look over.

darvanen → changed the visibility of the branch 3188383-reordering-overrides-drafts to hidden.

Setting isSyncing also seems to render drafts uneditable after the save, so ++1 for #13.

I've just realised we're on 5.0.0-alpha7 in case that makes any difference

Been testing ideas.

Saving the latest revision after saving the default revision stops the draft status from being lost, but it doesn't help with the "this content has been edited by another user" problem.

Setting the code which modifies the children to use setNewRevision(FALSE) doesn't stop the storage from creating a new revision.

I suspect the only way to truly remediate this problem porperly is to separate the hierarchy storage from entity storage. I wonder if there are any other performance improvements we could make along the way.

Alternatively perhaps we could ask the form to check for content locks before running (if that module is in place). That might be more of a project-level thing though.

Thoughts?

I made a mistake the first time around, have now fixed and here is the expected fail:

https://git.drupalcode.org/issue/drupal-3554149/-/jobs/7040119

Reviewed, this has been in production for a few months, and it's a one-line change to settings options.

Done

Running into this at the moment definitely still an issue. Seems like it makes a new revision but removes whatever identifies the latest draft, so you're left with the current revision being the published version if there is one.

I've provided a test that demonstrates the issue. I'm afraid I have to drop this for the moment.

darvanen → created an issue.

Are you across ✨ Add a [current-page:object:?] dynamic token Needs work ?

Closing due to no further comments/questions.

@sirclickalot are you using AJAX with the view? That might have some impact?

@bobi-mel welcome to the team :)

Sorry it took me a while.

@saurabh-2k17 it would be great if you could test the changes in the MR against your specific issue?

I think the tests are pretty comprehensive but I do really like to have confirmation against the original report where possible.

Pushed a(n accidentally) test-only MR and it fails as expected:

https://git.drupalcode.org/project/advanced_email_validation/-/jobs/6887330

Sorry I didn't see this when it came through, taking a look now.

Thanks @scottsawyer for this comprehensive request :)

I had a look around and I don't see any other modules which fit your requirements. I'm not super keen to extend this module directly to deal with individual email addresses but I'm more than happy to support plugins.

I'm thinking a plugin would require a validate function and at minimum an error message field in its settings form as config translation is a big part of this module's offering.

Then the block list of individual emails and the various ways people may wish to implement that could be a submodule. Thoughts?

but needs phpcs updates

This has gone through rigorous testing in an enterprise setting and is now live to the public.

That is quite the knot to unravel.

Perhaps a new token and deprecate the old one?

Just in case anyone's lurking here and interested - we did end up varying the cache prefix on cache clear by writing it to a file in private storage. This followed by garbage collection via cron allows us to see how quickly the cache refills and provides confidence that it is being used to its full potential. We've been using cache prefixes wiht multiple dbs in non-prod for years, this doesn't change anything about that.

I came here via https://www.pivale.co/resources/blog/drupal-sdc-vs-storybook

Feels like it might be worth putting that link on the module page (I'm not affiliated in any way)

@wotnak how do you reliably prevent name collisions in your manifest?

I'm working with the host to see if it's feasible to split up our environments, I didn't know that about queues, yuck, thank you. If we do succeed in separating instances then obviously a flushDB is the way to go, yes.

Thank you so much for your feedback, it has been very helpful, and I can see that if I do go with some form of cache prefix fix (much less likely now I know about the queue issue) it won't be useful generically, so I'll stick with the project-level implementation if it comes to that.

Drush command sounds good as a resolution to this ticket, might want to build the call into the client proxies though, cluster and standard clients do flushes slightly differently.

The problem I am facing is that the module invalidates cache entries by comparing them to a timestamp. This results in a huge amount of stale cache entries being queried and transmitted then discarded by the module, rather than a much quicker "not found" response (once you add them all up).
There are definitely areas in the project where improvements can be made but this change will considerably reduce request times for us.

You're right about state, that was my bad, I still hadn't thought through the full extent of the change.

We could do without the garbage collection, want it there so I can see if there's any headroom in our redis instance in various envs.

I've been hashing out this problem with @nick_schuch and he's had what I think is a great idea:

1. Create a random hash, store it in state and append it to the site's cache prefix so you have SITE_PREFIX:HASH:rest_of_key
2. On cache clear, change the hash and
3. Trigger an async garbage collection of all keys using the old hash using scan and unlink which are both non-blocking

I'm working on implementing that in our site, if you think it would stand a chance of being committed to the module please let me know and I'll create an MR :)

I see that failed testing, can someone help me understand how this is supposed to work? I'm guessing we continue to follow the convention of relative links within library yml files - how do you get your vite server to

1. find all the entrypoints
2. avoid namespace collisions e.g. two js/table.entry.js files

I've added a commit to generate some discussion here, it may end up being reverted and that's fine, but I do not understand anyone is getting their vite server to compile to an absolute location and keeping their library paths relative to the various modules/themes?

Because we're compiling at root, all our of custom libraries reference their entrypoints from root, which allows the vite server to scan the library files for entrypoints, and create a manifest which transforms those absolute paths into a dist path.

Am I missing something simple here?

As for the aggregation, it seems the only way to get Drupal to avoid doing that is to mark a file as type: external which directly conflicts with shouldBeManagedByVite(), so I'm going to open a core issue to address that because there needs to be another bypass in my opinion.

Agree that ->keys() is not safe it's plastered all over their documentation.

How about a similar approach using ->scan() then ->del() instead?

joachim → credited darvanen → .

We closed ✨ Settings overrides are still relative to library location Active in favour of this approach.

I've finally got around to trying it out and I have a problem.

All the assets are being optimised and the chunk addresses in the built JS are relative so the browser is throwing a heap of 404s.

How did y'all stop Drupal from optimising your built assets?

Also, I had to spend a long time with xdebug to figure out how the settings were supposed to work for this.

I agree, I was able to resolve the issue without making any changes to the Vite module.

Having to add ‘preprocessed: true’ to library definitions was the workaround for anyone finding this later.

darvanen → created an issue.

@tinny can you create an MR before setting to needs review please?

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

As requested giving this one a review.

I'm new to Threads but the loop makes sense to me from the examples given. If I have understood it correctly then the answer to my question on the MR should be 'none'. If that's the case then I'd say the logic here is solid (and we can remove a couple of unset commands).

My only other concern is I don't think we're testing the non-thread use-case? I know they were introduced in 8.1 and 8.0 is out of support, but what if someone is running a NTS binary or something?

I needed this before our D11 upgrade.

Here's a function-only patch → I made for 10.4 that works alright so far. YMMV

Not bad, thanks, appreciate the patch.
I think if we invoke hooks we'll do it in the AdvancedEmailValidator service rather than all the implementations of it in various plugins though.

Oh, I see this may be already addressed by inherting properties in the latest code, I'll update the module and come back to this when I have.

darvanen → created an issue.

@smustgrave I don't personally see why there would be, tokens are already a sprawling hierarchy with hundreds if not thousands of derivatives once you include the token module. Also once they're defined I think they're pretty lightweight.

I also see a core committer has interacted with this patch and didn't raise any concerns at the time.

The package has been published, this is ready to merge now.

darvanen → created an issue.

Pretty sure the bot is finished for D11

I think that's long enough :)

Patch doesn't apply any more

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?