🇬🇧United Kingdom @jamsilver

Hi Simon. A random assortment of thoughts:

My overall feeling is LCN is a fairly low-level module and working with the Dynamic Page Cache typically requires advanced tuning of configuration, customisations etc. It's an advanced use case and it's not clear to me what more LCN could do to support it.

Could add something to the Project Page/README about it though.

Have tested on D10 and (briefly) on D11.1. Suggest merging and fixing any bugs as folk find them.

jamsilver → created an issue.

jamsilver → created an issue.

jamsilver → created an issue.

jamsilver → created an issue.

I've done a quick test on Drupal 10.3 and a (very quick) test on Drupal 11.0.6 and it seems to work. I saw one thing on the MR that I think needs to be changed (see other comment).

jamsilver → created an issue.

Interesting. I find the module's current approach (i.e. /LANGCODE-COUNTRYCODE/where LANGCODE is configured per language and COUNTRYCODE is configured per country) to be good and sufficient.

(Well, I suppose I found it disorienting that the langcode url prefix used is the one on configured on /admin/config/regional/language/detection/url which doesn't feel like it should be related, but "good enough for the moment", I thought).

I'd be interested to hear more of your thoughts about the scenarios where prefix-mapping would get more complex than that?

Regarding making it pluggable, my first thought is: if someone needed to do prefix negotiation differently, then surely the natural step would be for them to write an alternative Language Negotiation Plugin? Is there value in language_country_negotiation adding an extra plugin-layer further to this? Put another way: what code would be left in LanguageNegotiationLanguageCountryUrl if it deferred logic off to a sub-plugin?

And (still regarding making it pluggable), my second thought is: If the scenarios you imagine are rare, then perhaps the use of a service ("PathPrefixHelper" in the current MR) is a good enough solution? After all, if another module wishes to adjust how prefixes are parsed (but in a way that is pretty-close-to-standard), then they have the option of replacing or decorating language_country_negotiation.path_prefix_helper. (If we code with the expectation that it may be decorated, perhaps the class should be renamed, final modifier taken off, and its methods restructured to make it more useful to do this kind of thing?)

My third thought: The MR removes length/format assumptions of both LANGCODE and COUNTRYCODE which makes them potentially quite flexible. The only other thing I would add is something on the language-country-url configuration form that lists every country/language permutation and allows a custom prefix to be provided for each. This would be fairly simple to implement, and would surely afford maximum necessary flexibility?

I've raised a MR that replaces PathUtility with a new PathPrefixHelper service that does the stronger check for configured languages/countries rather than a general regexp.

It incorporates the work in ✨ Compatibility Drupal 10.3^ Active , mainly so the pipeline passes.

jamsilver → created an issue.

I've updated gitlab-ci.yml so it only gets tested on Drupal 10.3, and made various other linting fixes. Pipeline passes now. Please review.

For expediency, I've just renamed this module's country code constraint. I've raised a MR that includes both the patch from #1 plus this change.

I applied the patch in #1 and it fixed the reported issue, but I'm seeing a subsequent error:

1. Go to the add Country form
2. Populate the field values, choose "GB" for Country Code
3. Submit

Expected results: The form is submitted and the Country is created.

Actual results: Validation error on Country Code field with message "The value you selected is not a valid choice."

It looks like core added a CountryCode constraint in 10.3 (CR here: [#3441838] ) which conflicts with the CountryCode constraint used in this module. Core's one appears to take precedence, but its validation isn't picking up the value properly from this field, and rejects in all cases.

Proposed Solution: The core CountryCode validator is a more specific validation (i.e. tests against the actual list of country codes). This module should probably use that.

The MR shows tests failing, but it's not the tests added by this MR (which are passing), so leaving at Needs Review.

It's a one-line fix, but I added Kernel tests around it to verify.

Note, if you wish to override to a completely empty array, you need to use the following syntax in settings.php (see https://www.drupal.org/project/drupal/issues/2990549 🐛 Impossible to config override a key's value to an empty array Needs work )

$config['remove_http_headers.settings']['headers_to_remove'] = new \EmptyIterator();

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

Committed and pushed to 1.0.x. Thank you for your contribution!

Looks like this was fixed last week: https://git.drupalcode.org/project/formstack/-/commit/1896f199a709fb633a...

Raised a follow up issue, requesting we store this node reference as a UUID rather than an ID to make it simpler to script the set-up and deployment of this module: https://www.drupal.org/project/cookiebot/issues/3414375 ✨ Store node reference via UUID rather than ID to aid multi-environment deployments Needs review

I've raised a Merge Request

jamsilver → created an issue.

Oh I've realised that the module maintainer fixed this issue directly on 2.x last month.

Rather than apply a patch, install this exact commit to get the better fix. For example:

php composer.phar require drupal/flexible_views:dev-2.x#a669a7

This patch does not work for me. It simply causes a Fatal Error when $fields tries to be used further down the method.

For me, the notice is associated with loading an attached Data Export display, since the style plugin is different. In this case the exposed form is irrelevant. Suggest an early return.

Merge Request raised.

jamsilver → created an issue.

Merge request raised.

jamsilver → created an issue.

I've raised a Merge Request with a first pass here.

In theory to remain backwards compatible we'd need to write an update hook that updates all user roles that have "access content" to add the new "administer views_save_search_filter searches" perm. This is not presently included.

Alternatively could bump the major version.

jamsilver → created an issue.

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

I've raised a MR that adds Move to Top / Move to Bottom buttons. It's not the full drag & drop solution, but it's not nothing either.

jamsilver → created an issue.

jamsilver → created an issue.

See attached patch.

jamsilver → created an issue.

Looks like https://www.drupal.org/project/content_roles → may be doing a very similar thing. If so, perhaps we should recommend am upgrade path to that module for D9?

This patch worked perfectly to solve the problem for ckeditor_resize 1.3.0

I had a situation with a unit tests that failed because it executed the same migration twice in the same page request. It wasn't enough for migrate_tools_sync state to be reset in the constructor.

I've created a Merge Request that adds the following changes to the patch in 22 (interdiff attached):

1. Reset migrate_tools_sync at the point of scanning through source rows to collect the IDs. This fixes my unit test
2. Factor that bit of code to a new method for readability.

I've also raised this as a Merge Request, rather than as a patch, since that is the workflow in place on this ticket originally. I raised a new one because things seem to have moved along quite a lot since the original was raised.

Tests fail, but the first error I'm seeing looks unrelated to my change. Is there a problem with the tests at the moment?

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

This has been working for us in a production codebase for a couple years.