Account created on 22 November 2010, almost 14 years ago
#

Merge Requests

More

Recent comments

🇦🇺Australia acbramley

Happy to close, I think splitting the update hooks now may be more disruptive. It's easy enough to remove the new attribute manually.

🇦🇺Australia acbramley

@anybody nope, I upgraded from 1.4. But that's exactly my point, I don't want the attribute so why is it being added to existing installs when menu_link_attributes_update_8002 is just about correcting labels and descriptions? Perhaps there should've been 2 upgrade hooks.

Just for my understanding: For what reasons are you removing it? I guess you don't need it and it fills up the UI?

I don't need it, it has no purpose in my site.

I don't think we need dedicated permissions.

🇦🇺Australia acbramley

I imagine this will end up being a meta with conversions being split up in some logical format, perhaps by module or by hook?

🇦🇺Australia acbramley

Echoing what I said back in #37 this is awesome to see this issue get so close, I really hope we can get to a state with the documentation that we can move some things to follow ups (that could block next minor) and get this thing in. Kudos to @nicxvan for the incredible effort recently!

I would love to test this on some of our projects that heavily use Hux and see what the migration path would look like. I'm hoping it's as simple as a namespace change on the attributes (and probably a class name change). I don't think that'll be possible though until the sites are on Drupal 11.

🇦🇺Australia acbramley

The upgrade path here added the container_class attribute even if it wasn't configured. This results in adding the attribute to sites that have removed it. Opened 🐛 menu_link_attributes_update_8002 adds container_class attribute if disabled Active

🇦🇺Australia acbramley

Thanks @theemstra, can we get a new stable release with this included?

🇦🇺Australia acbramley

Rebased and removed the cache contexts. From manual testing I don't see a local task on non parentable pages, let's see if anything fails.

🇦🇺Australia acbramley

This is still an issue https://git.drupalcode.org/project/entity_hierarchy/-/blob/3.x/src/Routi...

This effectively makes every node page uncacheable by DPC for users with access to the Children tab.

🇦🇺Australia acbramley

@smustgrave yeah I think that's a good idea, or perhaps just 2.1.x, since we're also dropping core versions here. That'd also mean we could ditch the upgrade path tests which are failing on next major

🇦🇺Australia acbramley

After a lot of faffing about trying to get previous major to work I realised that I'm dropping D9 support here anyway so it's going to fail... This is ready for a review.

🇦🇺Australia acbramley

acbramley changed the visibility of the branch project-update-bot-only to hidden.

🇦🇺Australia acbramley

acbramley changed the visibility of the branch 1452100-private-file-download to hidden.

🇦🇺Australia acbramley

One failure was related, the other didn't seem it.

🇦🇺Australia acbramley

Hey @japerry I've noticed this on a few issues of yours now but you seem to be forgetting to give credit for all the people that worked on these issues. Many people on this issue were not credited for their work. Hopefully this is just an oversight, would appreciate you fixing it up. Thanks!

🇦🇺Australia acbramley

Big +1 on this and agree with #12. The longer we wait to do this the larger the potential baseline as more and more code gets added, I know we're pretty good at asking for return types but this would enforce it.

🇦🇺Australia acbramley

Still needs more access based testing.

🇦🇺Australia acbramley

Generally in support of the feature but I think a bit more thought needs to be put into the access side.

The route requires edit access to the file, which is only allowed (with core) by the owner of the file. I also don't see much test coverage around access either?

Do we want to focus efforts on 🐛 File entity update is allowed only for user who has uploaded the file Active first?

🇦🇺Australia acbramley

You're right, I've dropped unsupported versions.

🇦🇺Australia acbramley

Re #97 - any chance you could remove that HTML and add it as an attachment instead to reduce the noise? It's quite hard to parse as is anyway

🇦🇺Australia acbramley

Nice catch!

This property is defined in UiHelperTrait, this may cause issues if the 2 traits don't exist on a test class right? (Although I don't know if that situation is realistic)

🇦🇺Australia acbramley

I've added a gitlab ci file to run tests, it seems like the expected values were for a much older version of the module. I've fixed them all up now.

🇦🇺Australia acbramley

I've just run into this with a client as well, this is on Drupal 10.3.2 (also reproducible on 10.2.6). It only happens on AWS environments and only when editing a node, not adding. Exposed filters work fine as well as exposed sorts.

The difference between editing and adding a node is down to query param length

When editing a node the query param is 2162 characters long
When adding a node the query param is 1938 characters long

This is referring to the number of characters appearing after /views/ajax? when clicking a pagination link in the media library on the above mentioned screens

🇦🇺Australia acbramley

Re #65 that happens currently (as in without the patch) and is intended functionality.

🇦🇺Australia acbramley

📌 Add Gitlab CI file Needs review should be committed first so we can start running tests against D11.

🇦🇺Australia acbramley

I am not seeing a lot of support for the changes here, and I disagree with #91 that it is analogous to Json::encode. With the Json::encode example we are passing preferred options to the PHP function.

With setTimeLimit we are also doing that (in its current state).

With the changes in the MR we are changing the behaviour by adding to the php ini time limit - this could be a huge behaviour change for people calling this function.

I think our best bet is deprecate + new function.

🇦🇺Australia acbramley

Pulled CI file from Add Gitlab CI file Active and added OPT_IN_TEST_NEXT_MAJOR

🇦🇺Australia acbramley

acbramley changed the visibility of the branch project-update-bot-only to hidden.

🇦🇺Australia acbramley

Working on these tests today, there's a huge amount to fix in there.

🇦🇺Australia acbramley

@greenhorn you do not need the patch, you need to clear cache.

🇦🇺Australia acbramley

I think we should also revisit removing the maxlength as per #38 and fixing the tests that failed because of it (as reported in #43)

🇦🇺Australia acbramley

This looks fine, but do we really need a test for this? Surely the maxlength property is tested elsewhere?

🇦🇺Australia acbramley

Came up in slack and the advice from @berdir was to deprecate instead. I agree as it's minimal code to get a list of revision IDs using an entity query.

🇦🇺Australia acbramley

I've also just found some very strange behaviour where the new JS themed messages are not being used for anonymous users. Both for webform submissions and even password reset submissions they're unthemed.

We're using the same method as Olivero so I'm not sure what's going on here.

When logged in everything works fine.

🇦🇺Australia acbramley

acbramley changed the visibility of the branch 3428529-automated-drupal-11 to hidden.

🇦🇺Australia acbramley

I've gone back through https://git.drupalcode.org/project/redis/-/commits/8.x-1.x/src/Controlle... and added all changes back to the ReportController instead of trying to fix the insane conflicts (due to the refactors to that controller on this branch).

If we can get this in without having to reroll again it would make my year 😅

🇦🇺Australia acbramley

That sounds likely, since config is owned by the site you can change the label in the view mode freely.

🇦🇺Australia acbramley

That is the label of a view mode that does not exist as part of this projects config. You can modify this in your own project, I'm not sure where it came from.

🇦🇺Australia acbramley

This looks outdated, feel free to reopen if needed.

🇦🇺Australia acbramley

This needs to be peeled back to just the relevant changes to be D11 compatible.

🇦🇺Australia acbramley

acbramley changed the visibility of the branch project-update-bot-only to hidden.

🇦🇺Australia acbramley

acbramley made their first commit to this issue’s fork.

🇦🇺Australia acbramley

The 2 failures were from the 2 lines, essentially testing some functionality which I can not see implemented anywhere.

The tests were testing that the existing alias based redirect was updated when the node had its alias changed. I personally can't see any code that would add that functionality, and IMO is not necessary. The whole idea of redirecting from a node alias would mostly around redirecting away from stale content easily (by using the alias). If we want to add automatic updates to existing alias redirects then we could do that in a follow up, especially seeing how long this issue has taken to get resolved.

Therefore, I've removed the assertions.

🇦🇺Australia acbramley

Not sure why this was closed, but I'd like this feature too.

Production build 0.71.5 2024