πŸ‡¬πŸ‡§United Kingdom @catch

Account created on 13 October 2005, over 18 years ago
#

Merge Requests

More

Recent comments

πŸ‡¬πŸ‡§United Kingdom catch

catch β†’ created an issue.

πŸ‡¬πŸ‡§United Kingdom catch

Install Drupal from scratch. eg. drush si -y standard
Using the UI, navigate to Admin > Extend
Enable the SDC Cache Issue module

What happens if the module is installed via drush instead of the UI?

I'm wondering if this is a race condition somehow between the sdc being rendered immediately after a form submission vs. the libraries being discovered - i.e. is the first SDC somehow rendered before the library exists as such on the system.

However I'm not finding in the code where the component libraries are actually attached (as in #attached) by component rendering to see how that happens.

πŸ‡¬πŸ‡§United Kingdom catch

Ah that is fair enough, let's explicitly postpone this then.

πŸ‡¬πŸ‡§United Kingdom catch

I think we still need feedback on #3 here, adding all that config schema seems like a lot.

πŸ‡¬πŸ‡§United Kingdom catch

the validation constraints only allow string, uri, file and image field types:

OK, but uri has exactly the same problem, it supports anything you can pass into Url::fromUri() which can includes entity:. As above, I don't think this is a new problem at all, but don't want people to be lulled into a false sense of security.

πŸ‡¬πŸ‡§United Kingdom catch

Retitling however I'm not sure this is the right place to do this.

Does the Url class not handle this already for example using ::fromUri() and then ::toString()?

πŸ‡¬πŸ‡§United Kingdom catch

Both MRs look good to me and I can't see anything else to do here.

πŸ‡¬πŸ‡§United Kingdom catch

Committed/pushed to 11.x, 11.0.x and 10.4.x, thanks!

πŸ‡¬πŸ‡§United Kingdom catch

@smustgrave but nothing in the actual twig template content needs to change, if people copy the entire docblock from core that is up to them. We don't add CRs for any other documentation changes, but people do copy and paste those around too.

πŸ‡¬πŸ‡§United Kingdom catch

We labeled this the Big Default Content Question: instead of referencing content entities and hence depending on them, and hence needing recreate them/ensure they exist on another site, we base64-encode files/images (the 95% use case) to avoid that problem altogether.

So to double check - what this does is to only base64 encode the image when configuration is exported, and then save it as a file entity when the configuration is imported - then on the site itself, the default value is still a file reference, not the actual base64 encoded image?

That bit seems superficially OK but I'm not sure I get the full consequences yet - if I export that config again (to config/sync) then import it, is it going to keep getting converted between base64 and and a file reference (presumably with a new file over and over again) or is the file uuid also stored so that can be used if the file already exists?

πŸ‡¬πŸ‡§United Kingdom catch

I'm confused - even though it only allows linking to a node, it still allows linking to an entity that's not a file, whereas the issue summary is specifically saying only files (not even media) should be supported because they can be serialized into YAML. If you can add a default value for a link that points to a node, the problem is there.

πŸ‡¬πŸ‡§United Kingdom catch

Committed/pushed to 11.x and cherry-picked to 11.0.x, thanks!

πŸ‡¬πŸ‡§United Kingdom catch

@yousefanbar if you configured the CDN, you need to check that, if your CDN is part of your hosting plan, you need to get in touch with them. Only you have this information, not us.

πŸ‡¬πŸ‡§United Kingdom catch

diffstat on that last couple of commits looks great, about 50 lines net removal, we don't have to add another template override to stable etc.

Nice to get this one over the line finally. Committed/pushed to 11.x, 11.0.x and 10.4.x, since this has UI/string changes I don't think it should go back to 10.3.x. Thanks everyone!

Did my best with commit credit but apologies if anyone was overlooked.

πŸ‡¬πŸ‡§United Kingdom catch

Same actual code changes as the 11.x MR so going to go ahead here.

Committed/pushed to 10.4.x, thanks!

πŸ‡¬πŸ‡§United Kingdom catch

πŸ“Œ Require Node.js 20 for Drupal 11 Active is in which hopefully simplifies things here.

πŸ‡¬πŸ‡§United Kingdom catch

Committed/pushed to 11.x and cherry-picked to 11.0.x, thanks!

We should backport this to 10.4.x but that will require a separate MR since it's not a clean cherry-pick.

πŸ‡¬πŸ‡§United Kingdom catch

Added a change record.

πŸ‡¬πŸ‡§United Kingdom catch

Yes this will save lots of trouble later on, let's do it.

πŸ‡¬πŸ‡§United Kingdom catch

One complaint - #theme links supports the exact HTML structure of the new template/theme hook so we can use that with a suggestion instead of adding yet another one-off theme hook. See #1804614: [meta] Consolidate similar twig templates and properly use theme suggestions in core β†’ for the general issue trying to fix this for existing one-off theme hooks across core.

πŸ‡¬πŸ‡§United Kingdom catch

Committed/pushed to 11.x and cherry-picked to 10.4.x and 11.0.x. I didn't backport to 10.3.x because it's a new constant in classes that contrib can subclass.

πŸ‡¬πŸ‡§United Kingdom catch

Added a release notes snippet for 10.3.2

πŸ‡¬πŸ‡§United Kingdom catch

The most likely way this could happen is if:

1. A site was originally installed on a version that had this setting, with config exported to the config/sync directory.

2. Site is updated to a version with πŸ“Œ Get rid of using 'views.settings':skip_cache in ViewsData Fixed applied.

3. Database updates are run, but then after that, the config is not re-exported to the config/sync directory.

4. Later on, config is imported - this would re-introduce the 'skip_cache' key that was removed in the update.

#2628144: Ensure that ConfigImport is taking place against the same code base β†’ would prevent step #4, but doesn't help sites that have already got into this state.

πŸ‡¬πŸ‡§United Kingdom catch

Bumping this after #3450203: Cannot save Views admin settings due to skip_cache error β†’ which looks like at least two sites on that issue got caught by the same situation (not confirmed yet). This is likely to become more of a problem as config validation gets applied - since config that was previously silently invalid will now be unsaveable in some cases.

πŸ‡¬πŸ‡§United Kingdom catch

Making sure we fully flush out the BC layer is directly related to this issue, and is partly why were on multiple issues related to the same problem.

It is but as @berdir points out, doing things the other way would likely have broken type hints, which is at least according to bc policy more of a break than breaking decorators (users of the service > decorators of the service, most of the time at least), and we definitely can't do that in a patch release.

Both the fix and the test coverage look good to me, let's try to get this into 10.3.2 (and 11.0.0)

πŸ‡¬πŸ‡§United Kingdom catch

The update path divergence issue just landed so this is unpostponed, but it will need to implement the new API (even if we only commit it to 11.1.x) so moving to needs work.

πŸ‡¬πŸ‡§United Kingdom catch

Went to publish the CR and noticed it already has the expected 10.3 version numbers listed, let's make our current and future lives easier and backport it all the way back. Also means the API will be available for contrib to reliably use earlier too.

πŸ‡¬πŸ‡§United Kingdom catch

Committed/pushed to 11.x and cherry-picked to 11.0.x and 10.4.x, thanks!

Theoretically we might need this if we backport an update to a 10.3 patch release (at least unless we're able to use a 103xx update number in all branches), however because this is a relatively big change I'm not backporting it just yet.

Don't think we need a release note here unless we backport to 10.3.x because there's no action for anyone to take, even contrib modules are relatively unlikely to need this at least compared to core.

πŸ‡¬πŸ‡§United Kingdom catch

I'm postponing this on πŸ“Œ The AJAX messages command should render messages using the twig template Active again - that will resolve the issue that people are experiencing here. Fine to continue discussing workarounds here, but to actually fix it, we need to change how AJAX messages get rendered.

πŸ‡¬πŸ‡§United Kingdom catch

(e.g. the string, link etc. field types).

This isn't the case for link, it takes a URI, which includes the entity:// format, which has an implicit, if not explicit, dependency on the entity being linked.

Not every entity reference ought to be allowed. Only entity references that target a File entity should be allowed, at least initially. Because files can reasonably be serialized into the exported config, but the same is not true for arbitrary entity references.

For all other default values or config -> entity references in core (blocks is one example), we require that the entity exists on the site in order to be able to export the reference. This has some inherent limitations - like having to create content on production, sync it locally, then create the config, then sync the config, but those limitations enforce some good practices too, and it does allow any entity reference to work. Why uniquely diverge from that pattern here and start (I assume) base64 encoding images into config?

πŸ‡¬πŸ‡§United Kingdom catch

Are you using a CDN to cache HTML pages? If so does it definitely vary by session cookie?

Are you using Drupal's internal page_cache module?

Can you post the request and response headers from the anonymous version of the page when you visit it as an authenticated user?

πŸ‡¬πŸ‡§United Kingdom catch

Committed/pushed to 11.x and cherry-picked to 11.0.x, thanks!

πŸ‡¬πŸ‡§United Kingdom catch

There's an interesting thing (depending on your definition of interesting) happening on packagist with Drupal 9 php stats:
https://packagist.org/packages/drupal/core/php-stats#9

At the end of 2023, PHP 7.4 was 16% of installs with PHP 8.1 at 65% and PHP 8.0 at 10%

Now PHP 7.4 is 29% of installs, PHP 8.1 is 53% and PHP 8.2 is 7.5%, PHP 8.0 down to less than 5%.

So PHP 7.4 usage is a higher percentage, while PHP 8.0 has been almost squeezed out by 8.1 and 8.2.

Drupal 9 usage has gone down by approximately 30-40,000 in the same time period as sites update to Drupal 10.

So what this shows is that the sites that were already on PHP 8.0 or PHP 8.1 have either updated to Drupal 10 already, or in some cases updated to PHP 8.2 (likely in preparation for a 10.x update). But then there is a rump of 9.x sites still on 7.4 that have neither updated their PHP version nor their Drupal version and are drifting further behind.

Some percentage of those 9.5 sites on 7.4 will update their PHP version and Drupal version at the same time.

This only gives us an idea of package installs though - there will be 9.5 sites that haven't run composer update/install at all and those won't register in the stats.

πŸ‡¬πŸ‡§United Kingdom catch

There's an interesting thing (depending on your definition of interesting) happening on packagist with Drupal 9 php stats:
https://packagist.org/packages/drupal/core/php-stats#9

At the end of 2023, PHP 7.4 was 16% of installs with PHP 8.1 at 65% and PHP 8.0 at 10%

Now PHP 7.4 is 29% of installs, PHP 8.1 is 53% and PHP 8.2 is 7.5%, PHP 8.0 down to less than 5%.

So PHP 7.4 usage is a higher percentage, while PHP 8.0 has been almost squeezed out by 8.1 and 8.2.

Drupal 9 usage has gone down by approximately 30-40,000 in the same time period as sites update to Drupal 10.

So what this shows is that the sites that were already on PHP 8.0 or PHP 8.1 have either updated to Drupal 10 already, or in some cases updated to PHP 8.2 (likely in preparation for a 10.x update). But then there is a rump of 9.x sites still on 7.4 that have neither updated their PHP version nor their Drupal version and are drifting further behind.

Some percentage of those 9.5 sites on 7.4 will update their PHP version and Drupal version at the same time.

This only gives us an idea of package installs though - there will be 9.5 sites that haven't run composer update/install at all and those won't register in the stats.

πŸ‡¬πŸ‡§United Kingdom catch

#2 crossed my mind, maybe in the process of doing this with arrayPIs, we might be able to come up with some API methods that would be useful as well. It's something we could do test by test, wouldn't need to be all in one go.

Adding πŸ“Œ Rework database update tests so we don't have to ship database dumps in git Active as a related issue, it's only vaguely related but the difficulty of maintaining text fixtures is very similar.

πŸ‡¬πŸ‡§United Kingdom catch

Yeah I have no idea how to solve automating this, especially with views_test_data because you can't even just install the module to get it usable, have to create state entries first, but good to have an issue open in case someone else has ideas.

πŸ‡¬πŸ‡§United Kingdom catch

Looks good but conflicts again.

πŸ‡¬πŸ‡§United Kingdom catch

Given this is a user-facing PHP error (and the preview just doesn't work in this case), bumping to major.

πŸ‡¬πŸ‡§United Kingdom catch

Good find. One comment on the MR.

πŸ‡¬πŸ‡§United Kingdom catch

Kicked off a pipeline to make sure tests are still green, still some unresolved feedback on the MR so moving to needs work - although reviews by more people would be really good here.

πŸ‡¬πŸ‡§United Kingdom catch

Try this:

composer update drupal/core drupal/quiz drupal/rules

However if that's the problem, you should ideally update quiz and rules before updating to 10.3, since this allows to update less things at once.

πŸ‡¬πŸ‡§United Kingdom catch

Class names look like this:

 339 | Class PM8                         |
| 340 | Class PJG                         |
| 341 | Class P5K                         |
| 342 | Class OX8                         |
| 343 | Class OOW                         |
| 344 | Class OGK                         |
| 345 | Class NUC                         |
| 346 | Class NRK                         |
| 347 | Class NM0                         |
| 348 | Class NJ8                         |
| 349 | Class NGG                         |
| 350 | Class NDO                         |
| 351 | Class NAW                         |
| 352 | Class N84                         |
| 353 | Class N5C                         |
| 354 | Class N2K                         |
| 355 | Class M2G                         |
| 356 | Class LWW                         |
| 357 | Class L7W                         |
| 358 | Class L2C                         |
| 359 | Class KR8                         |
| 360 | Class KG4                         |
| 361 | Class K28                         |
| 362 | Class JZG                         |
| 363 | Class JWO                         |
| 364 | Class JOC                         |
| 365 | Class JLK                         |
| 366 | Class JIS                         |
πŸ‡¬πŸ‡§United Kingdom catch

Committed/pushed to 11.x, 11.0.x, 10.4.x and 10.3.x respectively, thanks!

πŸ‡¬πŸ‡§United Kingdom catch

Was a bit hesitant because of the new class, but I think it's fine and better than shipping the bug for 11 months - backported to 10.3.x too.

πŸ‡¬πŸ‡§United Kingdom catch

Two further questions on this:

1. Right now XB only enables one XB field instance per bundle, but if unlocked slots are ever allowed on the non-default view mode, wouldn't it need to be a field-per-view-mode-with-unlocked-slots? Or all view modes stored in one field value with an additional layer of JSON nesting?

2. Note #3440578-80: JSON-based data storage proposal for component-based page building β†’ where I tried to think through 'special' view modes like search (indexing) and newsletter rendering - which usually show nearly the same content as the 'default' view mode but with certain differences like related content blocks or field labels removed. This gets very, very complicated with the current data model and unlocked slots. If we allow unlocked slots in multiple view modes, it could get even more complicated - e.g. say someone makes an unlocked slot in the teaser view mode for an article content type, and you then want that search indexed along with the unblocked 'body' slot on the article view mode.

I was also unable to find internal search indexing and multi-channel re-use such as e-mail in the product requirements.

πŸ‡¬πŸ‡§United Kingdom catch

Committed/pushed to 11.x, thanks!

I was about to ask for a backport MR, then I realised it would just be the same change with the first hunk of the diff removed, so went ahead and made that change locally for both 11.0.x and 10.4.x. Reclassifying as a bug report since this fixes performance issues.

πŸ‡¬πŸ‡§United Kingdom catch

Looking at those screenshots, I think this is probably a valid report, wondering if we could add hyphenate-limit-chars so that only actually long words get broken.

πŸ‡¬πŸ‡§United Kingdom catch

Can you post a screenshot of the issue?

πŸ‡¬πŸ‡§United Kingdom catch

Not sure what's best to do about the change record here, should we add a new CR alongside the ones that were already added for [#3427629]?

I think deprecation in 10.3 to exception in 11.x is OK given the config schema was invalid in these modules already.

πŸ‡¬πŸ‡§United Kingdom catch

Yes let's close this one, we can always tweak further.

πŸ‡¬πŸ‡§United Kingdom catch

See MR discussion. Uploading a modified views.view.test_serializer_display_field.yml from rest_test module and also the diff vs. 11.x

I had to call a couple of methods from ViewTestData, install views_test_data module, edit the view, save it, export it, run the test, get config validation errors, hack out the extra keys, import it again, and that fixes the initial test failure for StyleSerializerEntityTest.php (while introducing another one which I haven't looked into yet).

This confirms that these views exports are incredibly old and have not been maintained over time, so we are testing invalid views exports everywhere. Not really surprising but a shame this issue that's already been split from a 500+ comment two part issue is the one dealing with it.

I think we should open another spin-off issue to discuss how we could maintain these properly over time. Config validation might help, but it doesn't solve the problem of having to manually import, save, export the views.

πŸ‡¬πŸ‡§United Kingdom catch

@benjifisher see #3306434-21: [Config] Warning: Entity view display 'node.article.default': Component 'comment' was disabled because its settings depend on removed dependencies. β†’ #3306434-22: [Config] Warning: Entity view display 'node.article.default': Component 'comment' was disabled because its settings depend on removed dependencies. β†’ #3306434-37: [Config] Warning: Entity view display 'node.article.default': Component 'comment' was disabled because its settings depend on removed dependencies. β†’ for reports of performance problems.

Also @berdir above:

it's still a slow and memory heavy operation. There's no query we can run to find this information. Drupal has to load every single config object in the system into memory and loop over them.

I've seen sites with over 1500 config entities, very easy for this to take a second or more to do.

I think we could backport this to 10.3.x without the deprecation (the string addition seems OK - it'll be on a page that no-one has seen yet and is unlikely to visit compared to other string changes we could make).

πŸ‡¬πŸ‡§United Kingdom catch

I wonder if its also worth having an update hook to update the views config of existing sites if its unchanged?

We didn't do that with the original issue (which means a relatively small number of sites will be affected by the fact the shipped config wasn't updated in the original issue) because there's always risk of breaking customisations if the detection goes wrong etc. I think it's fine without. Should probably add a new change record encouraging people to update their admin/content views to get the new behaviour though.

πŸ‡¬πŸ‡§United Kingdom catch

I actually don't like "Drupal Plus" ... just "Drupal+" ;)

As the person who suggested Drupal+ (not after a lot of thought, just after some of the discussion about only using 'Drupal' in this issue), I agree with this. I could live with it in a URL, but it looks awkward otherwise.

πŸ‡¬πŸ‡§United Kingdom catch

@irsar any other ckeditor5 contrib modules or custom plugins? We need a bit more information to go on if you want help tracking down the issue.

πŸ‡¬πŸ‡§United Kingdom catch

@irsar do yo have ckeditor_font installed?

πŸ‡¬πŸ‡§United Kingdom catch

I didn't mean to set this to needs work, that was a mistake, but actually yes.

Also this is going to be soft-postponed on πŸ“Œ Handling update path divergence between 11.x and 10.x Fixed - we can't commit it until that issue lands and might have to use the helper method from that issue in this one, in case we ever did backport it to 10.4 for some reason.

πŸ‡¬πŸ‡§United Kingdom catch

Updated the issue summary.

πŸ‡¬πŸ‡§United Kingdom catch

This won't go back to 10.3, it might only get committed to 11.1 at this point.

πŸ‡¬πŸ‡§United Kingdom catch

This is breaking the ckeditor5 UI in 10.3. I think we should consider entirely rolling back and trying again. See πŸ› Replace LogicException with trigger_error in LangcodeRequiredIfTranslatableValues constraint RTBC for discussion.

πŸ‡¬πŸ‡§United Kingdom catch

#60 makes sense to me, the entity will be re-indexed but the translation won't.

Since this caused a regression before, committed to 11.1.x and cherry-picked to 10.4.x, thanks!

πŸ‡¬πŸ‡§United Kingdom catch

Moving this to postponed.

πŸ‡¬πŸ‡§United Kingdom catch

I think this is a good idea. We should open a follow-up for the default content import that comes with recipes to add some kind of created date offset key so that this could be done on import.

Probably also need an issue to convert Umami's default content to the new system.

πŸ‡¬πŸ‡§United Kingdom catch

Marking duplicate of πŸ› Incompatibilidad con Drupal 10.3.0 Postponed: needs info .

πŸ‡¬πŸ‡§United Kingdom catch

Add template variable deprecation.

πŸ‡¬πŸ‡§United Kingdom catch

I think this issue is better off in the core queue, needs architecture more than product discussion.

πŸ‡¬πŸ‡§United Kingdom catch

Added a trigger_error('...', E_USER_WARNING) so it now looks like:

>  [warning] User 6506 already has a learning path status on learning path 201. LPStatus.php:236

So noisy but not fatal any longer.

πŸ‡¬πŸ‡§United Kingdom catch

Committed/pushed to 11.x and cherry-picked back through to 10.3.x, thanks!

πŸ‡¬πŸ‡§United Kingdom catch

Another way to do this would be to checkout isSyncing() in presave and set it in the post update, that would also allow the update to complete but be a bit stricter at runtime.

πŸ‡¬πŸ‡§United Kingdom catch

πŸ“Œ Remove mentions of assert.active from .htaccess Needs work

Make sure you don't have assert.active set anywhere, including php.ini

πŸ‡¬πŸ‡§United Kingdom catch

Re-titling to make it clearer what the issue is, this probably needs to be moved to the contrib module.

πŸ‡¬πŸ‡§United Kingdom catch

catch β†’ made their first commit to this issue’s fork.

πŸ‡¬πŸ‡§United Kingdom catch

autoupdates makes this a non-issue, we can deprecate update module support for installing extensions soon.

πŸ‡¬πŸ‡§United Kingdom catch

I think we should deprecate multisite support in core in general.

Containerized hosting allows you to run multiple sites with one code base, but on isolated instances - this allows for deploying and running database updates for different sites at different times.

Multisite, unless you add a lot of scaffolding around it, requires updating the code base and running database updates on all sites at exactly the same time.

πŸ‡¬πŸ‡§United Kingdom catch

This is done, we have πŸ“Œ Replace Contextual Links BackboneJS usage with VanillaJS equivalent Needs work one remaining child issue (apart from deprecating toolbar when navigation is stable) to remove backbone too.

πŸ‡¬πŸ‡§United Kingdom catch

Yeah let's retitle it. The big one left here is πŸ“Œ Handling update path divergence between 11.x and 10.x Fixed

πŸ‡¬πŸ‡§United Kingdom catch

Probably time to update the title given its over a decade since we switched to Twig templates.

With SDCs we are adding more template-y things on top of the template-y things we already have, so it would still make sense to rationalise the twig templates we have.

πŸ‡¬πŸ‡§United Kingdom catch

This looks sensible and doesn't break any existing tests. However is there a minimal implementation of the form button being outside the form we could implement in a test module so that we could add coverage for it to BrowserTestBaseTest?

πŸ‡¬πŸ‡§United Kingdom catch

It's almost impossible to add this back - selenium doesn't provide the information back to PHP at all, there is not really a nice API even executing javascript against the browser to get it either. I had to do similar things for performance testing, and they were pretty evil, we can live without it until there's a nice way to get it, if that happens.

Committed/pushed to 11.x and cherry-picked back to 10.3.x given this is just a text change in the deprecation message.

πŸ‡¬πŸ‡§United Kingdom catch

If you're experiencing this issue, please confirm whether you have https://www.drupal.org/project/ckeditor5_line_height β†’ installed and whether uninstalling the module fixes things.

Production build 0.69.0 2024