🇬🇧United Kingdom @Driskell

driskell → created an issue.

Driskell → created an issue.

Driskell → created an issue.

Driskell → created an issue.

Driskell → created an issue.

@wim-leers Sounds like a plan! I will look at dropping a ticket across to Crop to not convert to absolute URI if CDN is installed. Then similarly to here, once core is updated, they can also drop their absolute URI completely.

Just in case anyone looks at my patch, I've hidden it because it needs work - it causes double encoding because the conversion of the absolute URI back to public:// doesn't decode path components so https://xxx/sites/default/files/file%20space.png becomes public://file%20space.png but actually, and this is probably ANOTHER thing Core needs to sort out, the original would have been "public://file space.png" - so effectively you end up with CDN creating a URL from public://file%20space.png of https://xxx/sites/default/files/file%2520space.png which of course then 404s... Good fun!

Heh yeh. It never has. The getExternalUrl() on the PublicWrapper encodes the target so if it contains a query string it becomes encoded in the path like a path component containing %3D. It seems it's been like this since it was originally written for the first Drupal 8. Likely it could be sorted but similarly I didn't have time to dig through Drupal on the potential breaks if there are any places assuming a path - and I'm pretty sure there are some - most places assume that public://xxxx means you can just access the local filesystem on public-directory/xxxx - and call realpath and all kinds on it. So the impact of allowing a query string on public://, could be, (I haven't dug too far), a very large BC break.

@Harish.04 I'm not sure I can help very much but the hook won't receive the absolute URL unless some other hook has expanded it. For an image it would receive public://2024-05/file.jpg as an example. So you can't really strip a prefix. If you're looking to alter what CDN is doing then I think you need to use a different method as CDN is not using hook_file_url_alter for a long time and instead overrides the FileUrlGenerator. Maybe previous in much older versions it did and in that case you may have seen the CDN url pass through the hook - but that is not the case anymore by design.

I've attached a POC patch.

Before using in production it needs a bit more checking and maybe improving and definitely tests. Concept seems OK - it just converts back what looks like a public URL into a public:// stream wrapper, which regardless of input should be safe as anything public:// is public. Flagging for review for the premise and can look at tests if the approach is OK.

@Harish.04

Latest version does see file_url_alter get called. It calls at the point that the CDN URL is generated rather than when the image is pulled from the CDN.

I did raise this issue on CDN now though but I think it's not your issue as the hook does run, it's just CDN refuses to handle an absolute URL: https://www.drupal.org/project/cdn/issues/3453869 ✨ Support absolute URLs that point to public stream wrapper files in FileUrlGenerator Active

Driskell → created an issue.

I think this still requires CDN change - as Crop has to add a query string - but core stream wrapper getExternalUrl actually encodes the path including query string so the query becomes part of the final path part public://styles/file.png%3Dquery which then breaks the final path. The image style module builds its URL by converting to absolute first then adding `itok`. It feels like a big core change to support query on stream wrapper paths. So I don't think Crop is doing anything wrong here.

I think CDN needs to check after the file_url_alter in canServe for the current base URI and support that as a valid path to convert to CDN. Otherwise I can't see this working well without Crop being "CDN-aware" which not sure it should be.

@wim-leers I've investigated a bit and I do actually think we've still got an issue.

public://xxxx stream wrappers cannot contain query strings. The core stream wrapper library will encode the path using UriHelper::encodePath so you cannot have a query string in these URLs.

The way that the core Image module generates URLs with `itok` is that it converts to absolute URI first and then appends it.

With CDN enabled - of course you end up converting to absolute - you have to. But at the same time - it does not support any other module that needs to convert to absolute on the current URI. I'm not sure how to solve this. I don't think there's any way for Crop to support CDN without explicitly having Crop aware that CDN exists.

Ideally, the CDN module would allow absolute URIs at the current BASE URI to be converted to CDN. That way if another module converts to absolute to add query string - like Crop does - during file_url_alter - it would all start working.

If you don't have any objections I'll get around to opening a new CDN issue to note that the FileUrlGenerator should support absolute URIs returned via file_url_alter, converting them back to relative with query string for processing.

@wim-leers Thanks! CDN now calls the URL alter - so that's one step forwards.

However that's the first part and as you noted in #3391254 the images won't be converted to CDN urls:

… I do see one major problem:

[...]

… that would mean that "cropped image" file URLs never actually would have been served from the CDN. But until #3179753: Improve far-future support: generate dynamically generated files automatically (f.e. image style derivatives) is supported, that wouldn't be possible either.

I think though this can be now easily worked on here. I can't see a specific reason why the crop module should change the URL to an external one. I can see in the comments in the code it's relating to the query string getting converted somewhere - so could be a workaround for some other issue that needs fixing first too.

      // If the URI has a schema and that is not http, https or data, convert
      // the URI to the external URL. Otherwise the appended query argument
      // will be encoded.
      // @see file_create_url()

I'll investigate this to see if perhaps it is an old issue that was already fixed, or if it needs a fix, and report back when I can. I'm not the maintainer but postponed until that info is available is way forwards.

> … I do see one major problem:
> [...]
> … that would mean that "cropped image" file URLs never actually would have been served from the CDN. But until #3179753: Improve far-future support: generate dynamically generated files automatically (f.e. image style derivatives) is supported, that wouldn't be possible either.

Yep, we actually patched this by calling file_url_alter from the CDN module via a patch but only absorbing the query string and then appending it at the end. Of course, not a viable public patch and very specific to our use case of a public CDN that proxy to the site! I think this issue of the URL been turned absolute should be fixed in the crop module now, to ensure it's compatible. There's no reason it should move to external URL that I can think of - or at least if there is a reason it can be dug up in that module.

Thanks for sorting this part of the issue though!

The approach by @larowlan seems very useful to me, as it means existing modules are unaffected and new ones can receive additional information via the new interface. Over time perhaps some more helpers can be added for moving temporary files too but this would be a good start and works great for me.

> If PHP has decided that a referenced object can be destroyed through GC before there are 0 references to it, then we have a problem we can not solve here.

With XDebug develop mode since 3.2+ if a function throws then XDebug will capture the stack trace along with all in scope variables in all frames. This means the transaction object never reaches 0 at end of function as XDebug holds a reference. Then end of script occurs and AFAIK XDebug doesn’t free - so end of script cleanup kicks in and picks at random to free (as far as PHP is concerned the only way the objects are still alive at this point is circular reference). So it’s less that they free before hitting 0 and more than they will never reach 0 so PHP just picks them off at random.

I kind of agree that this is somewhat unfixable and even if it was fixed it would be just too complex. The only way forward is probably to remove implicit transaction objects entirely or throw warning of no compatibility for develop mode in XDebug.

For 3.5 patch just needs adjusting to a MR and moving the change to DrushQueueWorkProcessor to the main module as it's no longer in the purge_drush submodule

In a way ideally Xdebug would make the exception tracking it’s doing opt-in or at least have an opt-out (the tracking keeps the transaction objects in scope if exception is thrown and caught so there’s a history of exceptions stored during debugging)

That would help for all use cases and if it’s runtime configurable allow Drupal as a framework to opt out until it removes reliance on the transaction object destructor to commit transactions.

Probably worth an IS update regarding the Xdebug impact and that in normal scenario transaction destruction is safe as it happens way before shutdown?

I agree with @mandrake that I think best approach is to move to explicit commit.

The patch I do not feel is feasible - making transactions silently auto commit would be dangerous to data integrity. We would then generate new problem scenarios that are difficult to diagnose. Also as noted everything then collapses into a single transaction that is auto committed at the end and I’m sure this has implications - albeit maybe rare - things like outbound API if they ever needed to call back for data they won’t see it yet if your request isn’t finished. Of course only in xdebug environments and unlikely in production but I am sure has been enabled even temporarily in production to diagnose.

@derickr It would be useful to drop that on the Xdebug bug tracker as it might be useful if others get there.
I'm also very curious why the last 8 exceptions are kept but that's perhaps not a discussion for here, and appreciate it is a bit of an education for me here as I'm keen to understand especially if things are meant to remain like this.

@derickr

> FWIW, I don't actually understand *why* you have these exceptions with references to these connection/transaction objects though. Unless you're using exceptions for code flow, which you surely did not expect?

Please see reproduction case in: https://bugs.xdebug.org/view.php?id=2222

There is nothing in code adding the variable "$destructing" to an exception. Just merely the presence of the exception is causing this. I think you'd be absolutely perfectly fine to store a reference to the exception and anything inside it. Just here it seems to be somehow grabbing everything in scope in each method in the call chain, which I am sure must be unintentional and I hope, fixable.

> and we need to commit it from the Connection object, if we cannot rely on the TransactionManager to exist.

I don't think you can assume the transaction is complete. Maybe you're in this destruction because of an error and it is incomplete. This could potentially commit a partial transaction and remove all usefulness of transactions.

> I think regardless of xdebug's behaviour we need to fix this in some way because we should not be relying on the order of object desctruction.

During normal execution there is no issue with destruction order from what I can see. When it goes out of scope and last reference is gone, it is destructed and freed, as per the PHP documentation. Ordering is only involved when you get to shutdown and things are left behind and that in itself, before we even worry about ordering, is a problem - such as in this case where Xdebug is preventing destruction of objects - or if somebody stored a transaction object somewhere.

Perhaps here it just needs a better exception - throw from connection if an open transaction and roll it back - in transaction just ignore missing connection as you'll know it will only be missing if you're in shutdown, and a transaction should never be destructing there. It'll make the error message clearer - you won't get an inTransaction on null - you'll get a "leaked transaction" message which is what Xdebug is doing here. Of course, doesn't workaround the Xdebug issue.

Issue with any change to destruction code is that I don't think you could ever workaround the Xdebug issue safely. If you think about it - if transaction objects are getting held open, every transaction is going to become one. Then you get to shutdown and collection, and you have absolutely no idea if the transaction was successful. And you still have the issue of one massive transaction when it should have been individual.

> More in general, I'm more and more convinced we should abandon implicit commits

I think this makes most sense.

@derickr Reading the statement I believe the way Drupal is currently using transaction objects is fine. It assigns one and then allows it to go out of scope to commit automatically.

The statement "The destructor method will be called as soon as there are no other references to a particular object" covers this. The key phrase here is "as soon as" which leaves nothing to assumption. This occurs the moment the method ends, or when the stack unwinds during an exception.

Having said that, perhaps readability is improved by not having this paradigm in Drupal as this maybe relies on more advanced knowledge of object destruction. Though I think one could argue the approach in PHP currently is implied and the most reasonable assumption, and only circular references are the advanced concept.

@alexpott All my tests were with PHP 8.1 so perhaps the behaviour changes in 8.2? In my specific case it was also during menu link building in cache clear so could be related to the menu and content in my case too.

@mglaman

I managed to dive into my specific example and did indeed find that a specific exception that was handled somewhere - if I changed how it handled it it seems to work again.

I did produce a reproduction for Xdebug on this: https://bugs.xdebug.org/view.php?id=2222
So I think this is likely an issue in 3.3.0 of Xdebug.

Can you try disabled XDEBUG?
I have this error but only on a test environment. Disabling XDEBUG resolves it so it seems XDEBUG is doing something in my case.

In general within PHP, __destruct should call immediately as a variable leaves scope (it has always worked like this for a long time AFAIK - and is related to the "when no more references"). AFAIK only a circular reference is deferred beyond the scope until a collection happens that can work it out.

It looks like perhaps something is causing an interference here and causing a transaction object to not get immediately collected.

Added more detailed description of problem and solution to IS

@smustgrave I updated issue summary. It was proposed resolution that was taken so I just dropped the not sure bit and I’ve updated other parts to reflect final status. Thanks for checking

Not sure the test failures are related.

@smustgrave I rebased it to 11.x and updated the MR. Not sure how to update the other one so I kept my existing.
It skips the test for SQLite since it will fail due to the linked issue. But it successfully fails without the fix on MariaDB / MySQL and passes after the patch.

Not sure the patch here would fix it as it only removes a route - and the issue here is missing plugin in cache - the existence of the original route shouldn't cause too much a problem I think?

Could be related to https://www.drupal.org/project/drupal/issues/3402447 🐛 RouteProvider pretends no routes if database connection lost, poisoning cache downstream Active
If the definitions for LocalTaskManager get regenerated whilst database is offline (maybe cache went offline too and triggered cache miss rather than exception) and when it finishes the cache is online it could store a poisoned cache which is missing some or all of the view plugins depending on when the database connection dropped.

Updated IS. Confirmed that it not just override views. It's all views providing a path. I think in my case with my PluginNotFoundException when it lists the plugins that were found it only contains a small number (5 in my case) of view plugins. The rest are missing. No consistency to this list.

Just flagging postponed as I am trying to resolve my assumptions on the views overrides - I think that's incorrect. So will update as I find time.

The RouteProvider issue is there it's just unclear whether it's tied exactly to my problem with PluginNotFoundException so just want to dig to make sure it nothing else. Will submit a patch later on for it too.

Driskell → created an issue.

Resolve review queue bot issue

Sorry - that last pushed commit was to fix the URL to be correct issue

I've updated the MR to include a skip for SQLLite and linked it to the issue about the double quoting that prevents the test from working.

I am putting this back into Needs Review because a failing test is impossible for SQLite due to a known issue (see #3349286). The test system online is MySQL so reproduces it correctly. You cannot test this locally with a SQLite database, it won't fail.

As for the other issues, #2371709 should be a follow on, it's a bit more involved. And hopefully at some point #3349286 can be looked at to fix the SQLite issue. But I don't feel either of these block this fix. The first improved it. The second is purely relating to local tests.

Driskell: Just calling the hook before processing seems to break things, potentially the query parameters added by Crop API cause issues.

Just tested this, and it's caused by Crop API's hook_file_url_alter converting the URL to FQDN. Taking just the query fixes the issue but doesn't form a nice contract or function in this module. So would need Crop API to not go to FQDN (but it does this to prevent encoding of the query string during conversion to external URL [1]) or would need CDN module to allow for the URL coming back from alteration being external even though the guards in the generate require it to be a stream wrapper.

[1] This is caused by Drupal\Core\StreamWrapper\PublicStream::getExternalUrl() URL encoding the path - so we get a bit deeper now in that in order to allow Crop API to simply add a query string and not convert to external URL - so that CDN can call the alter at the beginning of the function and it still convert to CDN - would need a core change here to separate query string from the path in a public:// scheme URI and not encode it. Not sure why it encodes the path at all though.

Driskell → created an issue.

What choice is there if Drupal core just no longer provides this API?

Sorry I didn’t quite understand the meaning. Do you mean choice for CDN? Or choice for modules like Crop API?

At the moment hook_file_url_alter is not deprecated. Perhaps if we assume it should be then Crop API should perform its own decoration of the generator before CDNs. I raised here though as it looked like a break - as the behaviour changes from what core’s own behaviour is. And the Crop API change would be a separate thing to sort independently.

Could you explain why this module needs to alter file URLs?

If the crop is changed for a file then the styles get flushed so they can be regenerated. For example if the style has a crop resize that results in a different aspect then changing the “crop centre” results in different views of the image within that aspect. The module adds a “h” parameter corresponding to a hash of the crop centre so that any caching proxy properly receives a different “h” for the different derivatives.

At the moment with CDN enabled since 4.x the hash disappears and any change to the crop after initial generation doesn’t reflect on the front end if the CDN is a caching proxy

Just calling the hook before processing seems to break things, potentially the query parameters added by Crop API cause issues.

Approach that seems to work is to call the alter on a copy of the URI, and extract the query arguments, and then use them in the returned URLs.

Driskell → created an issue.

+    // Also query without any default route parameters as they may not be
+    // present in the computed route_param_key.
+    $route_parameters_without_defaults = $route_parameters;
+    try {
+      $route = $this->routeProvider->getRouteByName($route_name);
+      foreach (array_keys($route_parameters_without_defaults) as $param) {
+        if ($route->hasDefault($param)) {
+          unset($route_parameters_without_defaults[$param]);
+        }
+      }
+    }

Shouldn't this be only removing route keys which MATCH the default? Otherwise, if you pass in a route key, which HAS a default, but is not the default, this will then match routes in the table which are for the default value, not the given value? Perhaps I'm misunderstanding the logic but it seems this might be only meant to be removing if the value is default, not if it has a default.

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

Driskell → created an issue.

I just discovered this after some hard waiting for reproduction scenarios.

Downloaded a specific CSS file and it seems that the fallback to file_info picks it up as assembler source text - delivering text/x-asm! Of course, this only happens for an uncached hit to an aggregation - subsequent will pick it up. So it just is annoying.

css_po4KdqlRZWF8CfE0QBHXRKXwhj22MhbNT8WiGLDtx9w.css: assembler source text, ASCII text, with very long lines (24366)

I haven't empirically tested this but my assessment of the core change in https://www.drupal.org/project/drupal/issues/2861552 🐛 Duplicate HTTP headers should be removed RTBC is that it only deduplicates headers configured via #attached, which are mostly Link headers.

The headers duplicated here are copied directly from the original response object to the new one and then pass through EventSubscribers that originally added them which then add them again. So I don't think that ticket fixes it. I also think it makes sense for Drupal to validate and deduplicate its own "header instructions" - but technically at the request/response level duplicating headers is normal and acceptable so it wouldn't make sense to reactively fix it at that level so not sure the fix would translate well

Test is failing because it doesn't seem to be installing imageapi_optimise - so in fact it's completely different tests failing to what is added.
I don't really know how to fix it but hopefully maintainer knows the issue. The test stuff in this ticket does pass.

Updated patch for latest version and applied as a Merge Request, and now testing.

Not seen it before but found an issue that was adding the table exception checks that would also resolve this: https://www.drupal.org/project/drupal/issues/2371709 📌 Move the on-demand-table creation into the database API Needs work

@andypost

At the moment it fails silently and corrupts cache during normal day to day running. The scenario of a writer creating the table and causing a valid read to then throw exception is true, but I think that would only occur once, when the table doesn't yet exist, and only in the perfect race between read and the very first ever write to the table, and it would recover completely on the second request and never happen again. Unfortunately it's difficult to quantify the real impact of that scenario but it seems to be small, and much smaller than the current issue's impact.

Technically speaking I think the correct fix is to detect the specific error of the table not existing. However, this might be complex across the different drivers and versions supported to detect the specific error message, and would require driver changes. If that was opted into it would be better I think as a post-task after this to get those driver implementation for throwing a specific exception for table missing, for use in this code, so we're never handling arbitrary exceptions and only handling the exception we intend to: Table does not exist. Arguably that would be easier to test too and wouldn't need the quoting change for SQLite.

What are your thoughts?

@smustgrave

I'm guessing the impact to all SQLite queries of a change to that would prevent this from being a bug fix in 9.5.

However, perhaps there just needs to be an acceptance that this test won't function properly with SQLite until the other issue is fixed so maybe there's a decision to be made here - in that the test will properly work on MySQL so the automated tests will work fine. It only will cause a problem for running locally with SQLite. Just I feel this patch has saved me many issues and is hugely beneficial - our incidents relating to cache corruption in this area are eliminated so far.

So perhaps the SQLite issue does not postpone this? So I'm U-turning as usual and returning to Needs review to get some thoughts.

Driskell → created an issue.

OK, tracked the quoting to double quotes back to https://www.drupal.org/project/drupal/issues/3126658 →
It seems not to have taken into account SQLite.
I checked and the ANSI quoting in Pgsql and MySQL should be fine as that forces double quote as identifier and it will never be string literal.
In SQLLite it actually is string literal if the column does not exist.

I don't know but... I am wondering if the above needs to be a separate issue as that's in a different module. So I'll get a ticket raised.
I've noticed there's some duplication of tests in the MR anyway so I'll push a cleaned up version - and I'll set the other ticket for SQLLite as blocking this one.

@smustgrave

I just ran the tests without the fix and received a failure. I then applied the fix and it worked again.
However, from a hunch, I hooked up SQLite as the test database, and yes it did not fail!

Bit of debugging later...

https://www.sqlite.org/lang_keywords.html
> If a keyword in double quotes (ex: "key" or "glob") is used in a context where it cannot be resolved to an identifier but where a string literal is allowed, then the token is understood to be a string literal instead of an identifier.

The query that is meant to fail because of a missing column actually now resolves to a "does this string equal this string" and is accepted as a string expression!
> SELECT 1 FROM "test57957719"."config" WHERE "collection" = :collection AND "name" = :name LIMIT 0, 1

Will have a think about a way to make the test fail properly on SQLite as well.

I think key feature missing that stops our migration is ability to select "Use Frontend Theme" (or select a specific theme). Ideally it wouldn't be a select a theme - but just use the active theme.

Overall it looks like Mime Mail might work better if it just had the above feature. At moment HTML Mail only handles HTML Mail well when you are using MIME. If that's off, all HTML is lost if you are using modules like Webform that support HTML. And if you are using the Mime option in HTML Mail, although it will work with Webform, it opens up issues with text mail, as you can't differentiate the two in the template.

@jedihe Ah quite right! I see now, thanks. I normally download the patch and commit it and hadn't considered that approach of direct URL. I'll edit my comment to clarify in case others see it.

Hi @anneke_vde
You can access the composer patch you need for an MR by visiting the changes tab and adding “.diff” or “.patch” to the end of the url: https://git.drupalcode.org/project/drupal/-/merge_requests/2851/diffs.diff