🇦🇺Australia @nigelcunningham

nigelcunningham → created an issue.

In my case, the issue was that the module makes the parameters for the subrequest be entity type, entity id and view mode. This prevents the Context code from recognising the contexts it would have found in the original request (in my case a group), so the content doesn't display because the views parameters that get their contextual arguments from the request context aren't satisfied.

I have a module that seeks to set the group context in situations where the context might not be obvious. I had it disabled because I was wondering whether I really needed the code. It makes things work again.

All of this leads me to the conclusion that view_mode_page needs a fundamental change to work properly - I think it should perhaps provide dynamic routes that directly invoke the entity view controller. Unfortunately I don't have the time at the moment to make a patch that does that.

I think I've just come across another variation of this issue in the bug I reported (exposed filter in a layout on a view mode page). I also traced the issue to the path returning the canonical URL for the parent path. I'm still investigating but I am currently wondering whether the right behaviour would be to define a route for each view mode page.

nigelcunningham → created an issue.

@weseze, the status you actually want is RTBC since you've said you've confirmed it works. Updating to that.

This is now fixed via other commits. Closing but giving credit.

I'm not able to reproduce this so assume it has been fixed. Closing the issue.

Apologies for my slowness in getting onto this. Looking at it now.

Thanks all of you for your work.

I was getting this issue when I failed to add '/' to the start of the URL. If I add the '/' and then clear caches (must be a cache tag issue there somewhere too), the menu links are correct. The cache tag issue could potentially be in my code - I'm using the menu token module too (my own update for D11).

Thanks for the reply; I'll keep it then :)

Thanks for the reply!

@yannickoo, would you use the function if it was fixed? I was thinking of removing it.

Ah, no need for me to do so - someone else has already made a MR. I'll just add the credit to the issue.

You're welcome. I'll seek to find time to do that (I know it doesn't take long :>)

I've confirmed that this does provide a better exception than the original and the code looks fine.

It would be better DX / UX to not have an exception at all but in this case it looks like that would require wider changes so I'll just mark as RTBC.

Fix a typo.

Added FAQ on avoiding the Request Entity Too Large error and overriding template yml in general.

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

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

Ok; I think I've gotten the artifact issue sorted - I needed to copy in the contents of one of the template files and manually insert extra excludes for the fonts that the vendor libraries bring in. Hopefully the maintainers of the gitlab scripts will eventually include support for overriding or extending the excludes and that will no longer be needed. For now though, it's at least getting further than it was and will hopefully successfully run everything.

Now that I'm aware of the other issues, I'll babysit their pipelines for a bit and hopefully even merge the PRs today.

Thanks again for bringing this to my attention!

Thanks for bringing this to my attention - not sure why I haven't seen the related issues. I'll pause the work I'm doing on another module and get this sorted.

Nij

Thanks for your message.

The line is only used if the site administrator writes and configures a function; it provides a way for them to implement something that meets their needs.

I've encountered the same issue, reviewed the patch, applied it and found that it addresses the issue correctly. Moving to RTBC.

Thanks!

For anyone coming by here later, the working directory that any created file's path is relative to is the webroot.

There's a patch implementing a deploy hook that works for me - I can download and restore a database backup, run drush deploy and see the Alerts module installed via config and then the colour fields fixed via the deploy hook. I don't propose this as a long term solution - it seems more likely that the data structure given when creating the terms in the install hook needs some tweaking.

I've found that the error happens because the color field value is empty, but I'm not yet sure why the colour doesn't get successfully saved. I've spent long enough on this, and will just add a deploy hook to my own module to set the values so I can get on with things. Sorry for not supplying a proper solution (I have a large database so reloading it to retest takes a while).

Oops. I accidentally replaced the issue summary. I couldn't find an archived version so I've put back a minimal summary. Apologies!

What I was going to add...:

I've come across this issue after installing the module on my dev environment (without problems), running a config export and committing it, then downloading a DB dump from production and doing a config import.

The vocab and the terms are created but the error is triggered because the colour field doesn't initially exist. After a couple of additional config import runs, the field exists and the error goes away but all the colours are configured as black.

It looks like the issue is triggered by not running the install hook, so you'd need to do a drush deploy rather than just a config import.

nigelcunningham → created an issue.

FWIW, I'm having similar issues because I'm implementing support for sending content updates via websockets on the tail of requests (looking at which cache tags were invalidated). The particular scenario I'm currently facing is that I've serialised the render array for a view when the user initially rendered the page. Now another request has modified the content and I'm seeking to re-render the view, but it has an exposed form and Formbuilder is seeking to check whether the session has a batch form state (line 253 of FormBuilder.php).

Fun!

I now have my code working fine, but have discovered a bit of a chicken/egg problem that might be useful to consider here:

The code access checker (DefaultMenuLinkTreeManipulators::checkAccess) can't usefully check access until after my contextualManipulator has run, because it hasn't resolved tokens yet. Would it perhaps be an idea to get rid of the distinction between the two types of manipulators and allow them to specify dependencies or weights?

I've successfully used the D10 version of this patch to make a simpler port of the menu_token module to D10. ( https://www.drupal.org/project/menu_token/issues/3525182 ✨ Simpler D10 port using MenuLinkManipulator patch Active ). I didn't find any probems with this patch while doing so.

nigelcunningham → created an issue.

Feel free, _nod.

I have to admit I'm not following what you're thinking so it might be better if you just go ahead and implement it.

Hmm, a test fails but it seems to be unrelated:

https://git.drupalcode.org/issue/drupal-3521884/-/jobs/5128162

The patch applies without modification to 11.x-dev. A branch has been created and pushed and profiling screenshots supplied above; resetting to needs review in the belief that I've done everything that was wanted.

Thanks, I wasn't aware of that.

For anyone else searching for a solution in future, I could tell I wasn't logged into Gitlab by going to https://git.drupalcode.org/. In the top right corner, it had a 'Sign in' link. I was already signed in on D.O so just needed to click that button. It then showed my profile and the sign in button disappeared.

gargsuchi → credited nigelcunningham → .

Here are screenshots without the patch:

and with it:

Oh, I've been reminded why I didn't add an 11.x branch: When I click on the "Create new branch" above (https://git.drupalcode.org/issue/drupal-3521884/-/branches/new?branch_na...), it's a 404 for me.

Ok. I identified the problem from profiling so that should be no problem at all. I'll add this in a little while.

nigelcunningham → created an issue.

Wow. Great to see this merged. Thanks all!

I've pushed a MR with work I've done to get responsive datatables working locally. This is a work in progress but I thought I'd share where I've gotten to - perhaps it will save someone else some hours of time. The composer.json isn't working for me - I needed to (in the root directory) run composer require with the following versions, using Asset Packagist support.

"npm-asset/datatables.net-dt": "^2.2.2",
"npm-asset/datatables.net-responsive": "^3.0.3",
"npm-asset/datatables.net-responsive-dt": "^3.0.3",

I also needed to explicitly set installer-paths (the Wikimedia merge plugin didn't work for me and is apparently deprecated anyway):

"web/libraries/{$name}": [
"type:drupal-library",
"type:npm-asset",
"type:bower-asset",
"npm-asset/datatables.net",
"npm-asset/datatables.net-dt",
"npm-asset/datatables.net-responsive",
"npm-asset/datatables.net-responsive-dt"
],

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

Thanks for the reply. That makes it clearer.

Regards,

Nij

nigelcunningham → created an issue.

Thumbs up, then, based on my superficial understanding :)

Is there any chance that you might go from not having query parameters on a URL to having them? If that's a possibility, I would add the cache context on all paths where that could happen.

Apart from that, I'm not seeing any potential issues from reading your description of what you're doing.

I like the point in #181 that the patch shouldn't cause a change in behaviour with pre-existing comment.

Re the support of aliases being an opt-in configuration option, that sounds like a good idea. Would it would be problematic to accept aliases in the add/edit form and optionally translate them to the unaliased version on submission, perhaps with an explanation there?

Hi @Berdir and @mark_fullmer.

I haven't read the whole issue so please forgive me if I'm restating arguments that have already been made. I did search for phrases related to my post but it would take too long to read the whole thing.

I would like to suggest that supporting aliases is important from a content editor experience point of view - we should be seeking to make Drupal as easy to use as possible. I can see there are probably issues around aliases being changed separately to redirect being created/edited, but I'd humbly suggest that providing the best CX should at least drive a reconsideration of your position here.

FWIW, I found this issue after dealing with a client reported bug that a redirect wasn't working as expected. They had a published node with the name they were attempting to redirect from. To get the redirect working, I needed to change the node name (and I unpublished it).

greggles → credited nigelcunningham → .

I do agree that it breaks backwards compatibility, but it's also true that the current interface is broken - caching information is discarded, which could perhaps be counted as a security issue. No point treating it as one though (I think) since this issue has been public for so long.

If that logic makes sense, I'd argue that the logic should just be fixed an dependant modules should be forced to update to match as quickly as possible.

gargsuchi → credited nigelcunningham → .

jct321 → credited nigelcunningham → .

Looks good to me, successfully tested.

The patch did the trick for me too.

Thanks for the feedback. Drupal 9 is no longer supported so I'm not concerned there.

I'll get on a make a release.

Fixed in the 2.4 release, which is coming out now. Thanks for the report.

The hook that adds the JS isn't being triggered. Try changing the function name in line 15 of leaflet_more_markers.module from:

leaflet_more_markers_field_widget_map_marker_widget_form_alter

to

leaflet_more_markers_field_widget_single_element_map_marker_widget_form_alter

I have experienced this too. As I write, I'm testing changing line 255 so that it checks element.length first. This looks so far like it makes things work for me:

if (element.length && element.get(0).checked !== value) {

I started work on updating the patch in earlier commits for merging, per SMustgrave's comment in #64, but have been asked to move on to other things, so I'm pushing what I've gotten done so far in the hope that maybe it won't go to waste.

Sorry - pushed branch to wrong issue fork.

nigelcunningham → changed the visibility of the branch 2575577-d11-comment-11-approach to hidden.

nigelcunningham → created an issue.

nigelcunningham → created an issue.

nigelcunningham → changed the visibility of the branch 2987537-custom-menu-link-11.x to active.

nigelcunningham → changed the visibility of the branch 2987537-custom-menu-link-10x to hidden.

nigelcunningham → changed the visibility of the branch 2987537-custom-menu-link-11.x to hidden.

FWIW, I've traced an issue in which saving a page was leading to a WSOD to stuff that seems to be related to this issue. In the site, which also uses menu_item_extras (though I don't think that's relevant), we were seeing an attempt at updating a node resulting in:

Drupal\Core\Entity\EntityStorageException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'bundle' cannot be null: UPDATE "menu_link_content" SET "revision_id"=:db_update_placeholder_0, "bundle"=:db_update_placeholder_1, "uuid"=:db_update_placeholder_2, "langcode"=:db_update_placeholder_3 WHERE "id" = :db_condition_placeholder_0; Array ( [:db_update_placeholder_0] => 56 [:db_update_placeholder_1] => [:db_update_placeholder_2] => 3e5d49ee-3087-4200-a9bc-caf63f09dd06 [:db_update_placeholder_3] => en [:db_condition_placeholder_0] => 18 ) in Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (line 817 of /var/www/html/docroot/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php).

Looking at what mapToStorageRecord in SqlContentEntityStorage is doing, I saw that $entity->bundle->value == 'main' but the code is trying to retrieve the bundle value from $entity->bundle->target_id. The difference seems to be due to ContentEntityBase having the following logic:

    if ($entity_type->hasKey('bundle')) {
      if ($bundle_entity_type_id = $entity_type->getBundleEntityType()) {
        $fields[$entity_type->getKey('bundle')] = BaseFieldDefinition::create('entity_reference')
          ->setLabel($entity_type->getBundleLabel())
          ->setSetting('target_type', $bundle_entity_type_id)
          ->setRequired(TRUE)
          ->setReadOnly(TRUE);
      }
      else {
        $fields[$entity_type->getKey('bundle')] = BaseFieldDefinition::create('string')
          ->setLabel($entity_type->getBundleLabel())
          ->setRequired(TRUE)
          ->setReadOnly(TRUE);
      }
    }

During the call above, the field definition is a string but the table mapping is for an entity reference. Clearing the caches makes saving work again so I guess something the bundle entity type is somehow getting set and the definition cached somewhere along the road. (I've spent long enough on this already, so won't chase this down further right now).

In any case, the patch helps, though it looks like I will also need to patch menu_item_extras if someone hasn't done that already.

I've updated the merge request for the 10.x branch. This included removing the last commit, which sought to revert the changes removing setting the bundle value.

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

Ok, I'll recheck. Thanks for the report.

When implementing this, please don't forget use cases outside of the USA. In Australia, we have some shorter six digit numbers (132 500, for example) and there are also the emergency service numbers (not necessarily 911 - alternatives I know include 000, 111 and 112.

This is happening - I'm doing a rewrite that is using Context + Media / Display Modes / Response Image Styles (optionally) / Image Styles.

I know it's been ages but thought you might like to know that a rewrite is in progress to use Context + Media. I'll close this issue as from here on, Media will gain a new 'Background Image' bundle with the Context module having support for a reaction type that lets you choose the Media item, display mode and other settings. Since I expect that people won't have a huge number of background images they'll be using, I've made it a dropdown for now. If that expectation turns out to be wrong, I'll implement a different widget.

I'm now rewriting the whole module to use Context + Media to implement the core functionality, and have gotten it going tonight. Plenty of cleanup still to do but wanted to thank you for the idea!

I have a rewrite in progress; the background image entity type won't be used when it's finished (Context reactions instead).

That will be because you're using PHP 8.3. I'm working on a rewrite at the moment and hope to have it available for testing within a week or two - it won't have that issue because I'm developing with PHP 8.3 too, so addressing those issues as part of the upgrade.

I'm working on a rewrite that will use Media support and image styles. I got it working tonight but still have plenty of cleanup to do, upgrade hooks to prepare and tests to write so it will be a while yet. Sorry for the delay but I hope you'll find it much better!

Hi.

Sure. Sorry for my slowness; too many things on the go at once (as always!).

Have you had a chance to test it with D11? I haven't done so thoroughly yet.

Further work is underway to get a D11 version out the door. I haven't dropped the ball :)

Perhaps it would help to use the Context API. For situations like running drush sapi-i, it might help in determining that rendering isn't recursive?

I'd be happy to submit it as "the" solution if that's what people want. Tests have been written; I could add them to the MR and credit my colleage, who wrote them.

And another tweak. Tests are coming.

Here's a small fix to the patch in 77.

I've tried HeaderUtils::parseQuery on the Australian Bureau of Meterology link http://www.bom.gov.au/cgi-bin/wrap_fwo.pl?IDN60140.html (which had two issues - the equals sign being added and the .html being changed to _html). It got through unmangled when extending the patch from #1464244 to replace parse_url in the line above.

Rather than stomping on the existing merge request, I'll attach my version as a patch in the first instance, and put it in the merge request if it gets favourable feedback.

Thanks!