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

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

Merge Requests

More

Recent comments

πŸ‡¬πŸ‡§United Kingdom catch

@johnwebdev the commit was made to 11.x, but we branched off the 10.3.x branch from 11.x just after it was made, so it's now deprecated in both the 10.3.x and 11.x branches.

What this means is:

1. 10.3.0+ sites will get a warning that core forum module is deprecated, and will be encouraged to install the contrib forum module.

2. We still need to actually remove forum module from the 11.x branch/11.0.0, that will happen in πŸ“Œ [PP-1] Remove Forum module Postponed .

πŸ‡¬πŸ‡§United Kingdom catch

Will this be a replacement for the performance checks where it assets the count between 2 values.

Yeah that's the plan. πŸ› Prevent session garbage collection during performance tests Active , which I just opened based on @Kristiaan's report, will cover the one known random test failure (found via this issue), but this issue will help flush any more out.

πŸ‡¬πŸ‡§United Kingdom catch

I haven't looked into whether this is viable yet, but we might be able to take a similar approach to #2463567: Push CSRF tokens for forms to placeholders + #lazy_builder β†’ - i.e. move the #id set in FormBuilder to lazily built placeholders.

They'd have to be uncacheable and rely on a static cache of IDs that have already been built, but we already rely on that in the current logic so it's not making it worse.

The case that this wouldn't cover is IDs added by AJAX, so is it really worth going to the trouble of a placeholder if it doesn't fix every case.

I think we should just switch to a random suffix here. Yes it's possible that people are relying on hard-coded IDs, but that code is already buggy since those IDs could have a counter appended, so it shouldn't stop us from fixing the core bug.

πŸ‡¬πŸ‡§United Kingdom catch

Added the early returns.

πŸ‡¬πŸ‡§United Kingdom catch

Started an MR. Turns out we do pass empty arrays to these during tests so they need the same treatment as isValid(). Have run out of time - we still need to do the same for the trait methods.

πŸ‡¬πŸ‡§United Kingdom catch

Good point, not sure if those are also polluting the grafana traces but even if they're not it makes some things easier to follow, opened πŸ“Œ Return early from more cache tag operations Active .

πŸ‡¬πŸ‡§United Kingdom catch

Reviewed and manually tested - looks like a good change and working well, RTBC.

πŸ‡¬πŸ‡§United Kingdom catch

I did originally think about a CR, but now we've made the interface internal I think we should explicitly not have one, and try to remove it again if we can rely on session auto saving.

πŸ‡¬πŸ‡§United Kingdom catch

I've read it should be possible to use a Docker-based solution to run Linux with Apache or Nginx and PHP on a Windows host. In such a scenario, Drupal may not be relying directly on Windows, but it would still be running on a Windows server.

This would still be supported more or less by default because we support running Drupal on linux in docker, and what exactly docker runs is not usually taken into account. As you say:

In a Docker-based scenario, chances are, any security issue would be more related to the architecture within Docker rather than the host anyway.

Agreed with this - I can't think of a situation where there'd be an issue, especially a security issue, specific to Drupal on docker on windows, as opposed to a generic Drupal issue or a generic Docker issue.

However dropping support for directly running on Windows in production also means dropping support for apache on windows, not just IIS on windows - just to make that clear. We'd still accept bug reports from people using windows + apache (or even IIS) for local development, but any security issue would be made public as long as it's really windows-only, and we'd stop shipping the IIS file.

I tried to update the issue summary to make this a bit clearer, although the wording probably needs improvement.

πŸ‡¬πŸ‡§United Kingdom catch

It doesn't save any real database queries, we already check that there is something new to query in CacheTagChecksumTrait::calculateChecksum and similar places, so it's mostly removing noise when profiling (either traces in grafana tempo or less function calls to look at in xhprof and similar too) and a small micro-optimization.

πŸ‡¬πŸ‡§United Kingdom catch

What I was thinking of when I wrote 'is a specific regression test needed' is we have in some cases written one-off tests that are very specifically written for 'is this bug that we used to have not there any more', and are not 'does this system work as it's intended to', and that over time makes the overall test suite harder to maintain, and reduces our test vs. coverage ratio (i.e. we've have several cases of multiple tests testing more or less exactly the same thing, added years apart in different issues). But I think the rewording in #105 is plenty here and covers that situation well enough.

πŸ‡¬πŸ‡§United Kingdom catch

Went ahead and implemented #9. I also noticed that we were still sending some cache tag queries to grafana, so moved that logic to a helper so it's the same in both places we check.

πŸ‡¬πŸ‡§United Kingdom catch

Looks good to me, will commit later if no comments since this already landed once.

πŸ‡¬πŸ‡§United Kingdom catch

Thinking about it - we should call the child class so it's a proper decorator, but skip logging because we know there's nothing to log. Probably needs a separate comment. The empty cache tag operations create a lot of noise in grafana tempo, not having them there will make it easier to find things we need to change.

πŸ“Œ Track cache tag queries separately in performance tests Active just landed which means we can do that change here now, it'll mean also adjusting the assertions we just added.

πŸ‡¬πŸ‡§United Kingdom catch

I think the change here is good, but also just realised we could do the same early return in the decorator in πŸ“Œ Track cache tag queries separately in performance tests Active too.

πŸ‡¬πŸ‡§United Kingdom catch

Closed the draft MR, rebased, added the follow-up to match the @todo, and updated the issue summary. No real code changes apart from different numbers in StandardPerformanceTest, so moving back to RTBC.

πŸ‡¬πŸ‡§United Kingdom catch

Committing the test change from needs review given HEAD is failing and the adjustments are minimal, agreed the test is not fun.

πŸ‡¬πŸ‡§United Kingdom catch

The new way of immediately adding messages via ajax means that you're more likely to get duplicate messages on the screen because you don't benefit from automatic deduping by the messaging system.

Just to say a caveat to this is that if you submit a form, the messages are added to session, and the next page renders them (the most likely way to get lots of duplicate messages), then the deduping logic will still kick in, because any messages in session are still rendered via PHP with the placeholder, not via AJAX. So this would only happen when messages are rendered after the messages placeholder itself is rendered (i.e. from another placeholder).

πŸ‡¬πŸ‡§United Kingdom catch

This broke pgsql tests, should've thought to kick off an MR run on the other databases. https://git.drupalcode.org/project/drupal/-/jobs/874457

Not rolling back yet since it's explicitly the pgsql transaction coverage that's failing so maybe we just need to fix an assumption? But if it's tricky we should roll back and re-commit with that fixed.

πŸ‡¬πŸ‡§United Kingdom catch

I wrote some code for one of the MRs here but none of it got used in the final version so I think I'm fine to commit. I think this is the best we can do, it makes things more robust and it makes the weirdness of the current implementation more explicit with the workaround, which isn't bad thing even if it's not pretty.

Committed/pushed to 11.x, cherry-picked to 10.3.x, and the separate MR to 10.2.x

I haven't committed the 10.1.x MR to 10.1.x, it's in security-only mode so we'd need to do a 'bonus' patch release or bundle the commit with a security release for it to actually be useful to people. Tempted to leave things there and people can apply it manually if they need it, so marking fixed, but if someone feels really strongly about a backport to 10.1.x, please re-open.

πŸ‡¬πŸ‡§United Kingdom catch

For when to actually do this, probably something like this:

1. Deprecate modules on 10.x (actually happening as of this week!)
2. Remove deprecated modules and old updates from database dumps and update tests etc. on the 11.x branch, in a single issue if we can. That can probably be this issue.
3. Actually remove the deprecated modules from 11.x

With 1.5 figuring out when it's safe to do #2 so we don't have to do it more than once, just have to pick some kind of cut-off point where we don't expect to add new update hooks, or we commit to leaving those update hooks in the 11.x branch.

πŸ‡¬πŸ‡§United Kingdom catch

One small and one slightly larger question on the MR - would be good to document this in the issue summary and we probably need an explicit follow-up to track removal.

This is one where I think we should deprecate for removal in Drupal 11 since we're maintaining a non-trivial bc layer and at worst this will affect shipped config.

πŸ‡¬πŸ‡§United Kingdom catch

From looking at a couple of the test failures they look like ones we'd probably have to deal with when we move book out of core - like comparing default config etc. if that's the case then I think it makes sense to do it this way.

πŸ‡¬πŸ‡§United Kingdom catch

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

πŸ‡¬πŸ‡§United Kingdom catch

Since all I did was convert the patch to an MR, I think I can RTBC this. Updated the issue title and summary.

πŸ‡¬πŸ‡§United Kingdom catch

Converted the patch to an MR. Given #18 was green, we might have just fixed this elsewhere without removing the @todo.

πŸ‡¬πŸ‡§United Kingdom catch

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

πŸ‡¬πŸ‡§United Kingdom catch

If modules can provide default config for themes, we could move the block over and not the CSS, but... can they even do that??

πŸ‡¬πŸ‡§United Kingdom catch

Yep agreed here. Also doesn't stop us renaming it later if we come up with something, but I think 'dependent/dependency' is too ambiguous so it would need to be renamed to something very different.

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

πŸ‡¬πŸ‡§United Kingdom catch

I think a similar example is where performant got added to the Cambridge dictionary due to its technical usage even though it was not really an English word before that, although in this case nod_ pointed out it's also a real French word that translates to the same meaning as its used in English. So I think we can treat it as a neologism rather than a mis-spelling. I think if we were going to try to change this in the states API we might want to completely change the vocabulary (no idea what to, but something without the ambiguity of dependency/dependent/dependee).

πŸ‡¬πŸ‡§United Kingdom catch

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

πŸ‡¬πŸ‡§United Kingdom catch

The API for hrtime() is different, see https://www.php.net/manual/en/function.hrtime.php so can't just swap on for one but need to handle the return value differently too.

πŸ‡¬πŸ‡§United Kingdom catch

I opened the follow-up for hrtime in πŸ“Œ Use hrtime() instead of microtime() for timing things Active so removing the tag.

We're executing an increment in a place where you're supposed to return a value and I'm not sure if php will keep allowing that for all eternity.

It seems like the worst they could enforce is explicitly returning NULL?

I personally find the match statement more readable than the switch statement (no break or default to worry about, which can always theoretically go wrong if one gets misplaced).

πŸ‡¬πŸ‡§United Kingdom catch

Rebased, accepted the suggestions, and updated the comment. Since all those changes were minor, moving back to RTBC.

Also removing the 'needs change record' tag since that's done now.

πŸ‡¬πŸ‡§United Kingdom catch

Committed/pushed to 11.x, thanks!

πŸ‡¬πŸ‡§United Kingdom catch

@daniel.bosen has tried the patch out on Thunder and has run into a problem with the ConfigInstaller::installDefaultConfig() change because Thunder uses the config_selector module. That module decorates the config installer and changes the behaviour of installDefaultConfig().

Could we introduce a .info.yml key that forces a container rebuild on install? I think that would be OK in a minor release with a change record. Or worst case we could assume it's set in 10.x (so the behaviour doesn't actually change), then flip it in 11.x so that the performance improvement kicks in.

πŸ‡¬πŸ‡§United Kingdom catch

Should be ready for review now.

πŸ‡¬πŸ‡§United Kingdom catch

The issue summary and title say the opposite of what the patch does, could use an update.

πŸ‡¬πŸ‡§United Kingdom catch

Opened πŸ“Œ Track cache tag queries separately in performance tests Active which should hopefully make things easier here once it's in.

πŸ‡¬πŸ‡§United Kingdom catch

The MR is up, I'm seeing variations though in both the isValid and checksum counts, in what might be both directions, when running on gitlab - this potentially explains the query variation we're seeing elsewhere and might mean we can use exact assertions for database queries once this lands. Need to get to a green MR though..

πŸ‡¬πŸ‡§United Kingdom catch

Let's add a comment explaining what's happening there.

πŸ‡¬πŸ‡§United Kingdom catch

Reverted, let's extract the fix from that issue to here.

πŸ‡¬πŸ‡§United Kingdom catch

Committed/pushed to 11.x, thanks!

πŸ‡¬πŸ‡§United Kingdom catch

Went to commit this but it needs a rebase for phpstan-baseline.neon

πŸ‡¬πŸ‡§United Kingdom catch

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

πŸ‡¬πŸ‡§United Kingdom catch

Also this fix only applies to the default connection via the database service, I assume anyone doing anything with other connections is on their own.

We could probably loop over all database connections and commit their transactions, but it'd need a diversion from the service approach then.

πŸ‡¬πŸ‡§United Kingdom catch

Moving the jQuery UI dialog and autocomplete to 'should haves' - they don't block jQuery 4 (as far as I can tell), so our main issue is jQuery theoretically dropping support for them before the EOL of Drupal 11.

πŸ‡¬πŸ‡§United Kingdom catch

Committed/pushed to 11.x, thanks!

πŸ‡¬πŸ‡§United Kingdom catch

Double checked in CronForm whether it includes the cron status/last stufff, and it does. This looks good to me.

πŸ‡¬πŸ‡§United Kingdom catch

Committed/pushed to 11.x, thanks!

πŸ‡¬πŸ‡§United Kingdom catch

One question on the MR, everything else looks good.

πŸ‡¬πŸ‡§United Kingdom catch

Looks fine except for one change I didn't understand.

πŸ‡¬πŸ‡§United Kingdom catch

@dww this is only about the core packaged tarballs like https://ftp.drupal.org/files/projects/drupal-10.2.3.tar.gz that include /vendor code and are generated by the packaging script. drupal/core, drupal/core-recommended etc. would still be in .tar.gz format but they're only pulled via composer and not exposed for direct download and install. Updated the title to make this a bit more explicit.

πŸ‡¬πŸ‡§United Kingdom catch

#2495087: comment_entity_storage_load() is too expensive on cold caches β†’ is still open but we don't need this meta to track that.

πŸ“Œ Add a cache prewarm API and use it to distribute cache rebuids after cache clears / during stampedes Needs work is new and attacks a lot of the same ground in a different way.

πŸ‡¬πŸ‡§United Kingdom catch

We're still doing route rebuilds at the end of the request where they're non blocking, so closing this one out.

πŸ“Œ Add a cache prewarm API and use it to distribute cache rebuids after cache clears / during stampedes Needs work is more comprehensive and applies a similar decoupling concept for a lot of different rebuilds though.

πŸ‡¬πŸ‡§United Kingdom catch

This never went anywhere, closing out.

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

We did this (and related issues) instead: πŸ“Œ ConfigFormBase + validation constraints: support composite form elements Needs review .

πŸ‡¬πŸ‡§United Kingdom catch

Bundle creation is still per-entity type and there's no plans to change it, if there were, we should start again.

πŸ‡¬πŸ‡§United Kingdom catch

Taxonomy terms can have custom fields now so there's zero need for this in core as such.

πŸ‡¬πŸ‡§United Kingdom catch

Shared tables are no longer supported, per https://www.drupal.org/node/2768219 β†’

πŸ‡¬πŸ‡§United Kingdom catch

The assumptions in the MR are wrong, there's no benefit to doing this. We've introduced Fibers elsewhere though.

πŸ‡¬πŸ‡§United Kingdom catch

@dqd https://www.drupal.org/docs/system-requirements/browser-requirements β†’ and the answer to your specific question is since 2019 after many, many years of discussion in #2390621: [policy, no patch] Update Drupal's browser support policy β†’ . It's an off-topic discussion for this issue - as long as the change meets the browser support policy, it's fine, but that does include Firefox ESR.

A polyfill is possible here, but adding 10.4/July 2024 to the title since I think we could also just wait until 10.4 is open for development and do it then, which would mean we actually release the change in December 2024, 10 months from now, which gives lots of people time to update Firefox ESR from when it comes out. We should only add the polyfill if we really want this to land in 10.3/11.0

πŸ‡¬πŸ‡§United Kingdom catch

I opened an issue against jquery-form asking about a new jQuery 4 compatible release: https://github.com/jquery-form/form/issues/616

Just in case, but if that goes unanswered within the next week or two, I think we should proceed here given there's not much activity there in general any more.

πŸ‡¬πŸ‡§United Kingdom catch

I think the main remaining things are:

1. We should open issues for taking things out of state and into key/value directly which are accessed extremely rarely. The new development settings stuff that's only on container rebuild / form save are the obvious ones, there are probably more though.

Also an issue against simple_sitemap would make sense too.

2. Decide whether the changes in core/modules/language/tests/src/Functional/LanguageNegotiationContentEntityTest.php are acceptable here with a follow-up to move the test modules to use key/value, or whether we should refactor those test modules to use key/value in this issue. This also goes for the various test modules used in the tests that are getting WaitTerminateTestTrait added. I could go either way here - refactoring the test modules adds scope, but it'll be less churn because we then wouldn't be adding WaitTerminateTestTrait and removing it again.

Tagging needs follow-up since we need that for #1 even if we need to make a decision on #2.

πŸ‡¬πŸ‡§United Kingdom catch

Committed/pushed to 11.x, thanks!

πŸ‡¬πŸ‡§United Kingdom catch

Moving this to needs work for the outstanding feedback on the MR.

I'm also wondering if the changes that are outside Editor module could be done in their own issue before this one?

πŸ‡¬πŸ‡§United Kingdom catch

Those look like real failures. Reverted from both branches.

πŸ‡¬πŸ‡§United Kingdom catch

Added a change record; https://www.drupal.org/node/3421405 β†’ and committed/pushed to 11.x, thanks!

πŸ‡¬πŸ‡§United Kingdom catch

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

Removing the 'major version only' tag because it's fine to test with this combination on 10.x too.

πŸ‡¬πŸ‡§United Kingdom catch

The MR pipeline failed three weeks ago, I just re-ran it and it failed again.

πŸ‡¬πŸ‡§United Kingdom catch

Committed/pushed to 11.x, thanks!

πŸ‡¬πŸ‡§United Kingdom catch

Moving back to needs review until the image name is resolved - unless it can be changed on the image side with bc so that it doesn't break the new job.

πŸ‡¬πŸ‡§United Kingdom catch

Committed/pushed to 11.x, thanks!

πŸ‡¬πŸ‡§United Kingdom catch

I can't remember the exact slack conversation with @daffie but my general feeling is if we're going to start casting database results, we should open a meta issue and also do that for nodes, taxonomy, comments, menu link content etc. etc. since they all have the same problem for IDs. There'll then be other base fields that could also be more strictly typed etc. so this is a not a small one-off task but a significant project that needs to be coordinated across core entities.

But... I also think the docs should reflect reality - whether that means casting here as a one-off/first of the meta issues, or updating things to reflect reality then changing them again later, I don't have a strong opinion.

πŸ‡¬πŸ‡§United Kingdom catch

This has been ready to go for a while - can someone do the honours of the actual documentation update so we can mark this fixed?

πŸ‡¬πŸ‡§United Kingdom catch

I looked at the diff in https://git.drupalcode.org/project/drupal/-/commit/6bd3ad841f346d650ace3... and I also can't tell what's left here.

::handleFileUpload() creates violations, these are then formatted into a messenger message here: https://git.drupalcode.org/project/drupal/-/commit/6bd3ad841f346d650ace3...

The issue summary is saying that instead of listing the errors, we should log them. The errors we list now are validation constraints, not the raw message, so I think that's fine.

However, it looks like whereas we used to cover UPLOAD_ERR_NO_TMP_DIR as an explicit case in FileUploadHandler::handleFileUpload() https://git.drupalcode.org/project/drupal/-/commit/6bd3ad841f346d650ace3... that wasn't handled in the new code, and I can't see anywhere else in core that it's handled. Symfony's FileValidator does handle it explicitly but I don't think we're using that.

Given all that, if the above is correct, which I'm not 100% sure it is, then I think what we're missing is logging the full range of errors we used to throw exceptions for, but now only show a handful of violation messages for.

πŸ‡¬πŸ‡§United Kingdom catch

+1 from me too, if it's in scope to add to the docblock, it's also in scope to remove it altogether given the new coding standard. I co-incidentally ran into this today on πŸ“Œ Use MessagesCommand in BigPipe Needs work and took the approach here.

Production build https://api.contrib.social 0.61.6-2-g546bc20