@alexpott, yeah - you are 100% right. I thought that those IDs are large numbers .. for whatever reason.
Anyways, thanks for the review. Would be greta to have it in soon(ish)
I've added revision ID + language to the keys (not sure those are needed actually, but they don't harm I think)
taran2l → changed the visibility of the branch 3531245-create-drupalnl-config to hidden.
@jonathanshaw I was working/looking at 4994 (I guess you have MR activity turned off so you do not see any new commits )
it requires a manual rebase now, and one more test is not optimal. I plann to take a look today and hopefully move to needs review
So, I gave it some time and here are my updates:
- Webform is no longer an issue, actually it never was. There was a bug in theme hook implementation, which has been fixed in 🐛 Incorrect use of 'render element' for webform_submission_data Active
- Some tests were NOT failing with the existing approach. However they should have. Apparently, tests were affected by the caching. I have changed tests to use a non cacheable view mode. So they correctly fail with the existing approach and are green with the new one
- While running test locally I saw that the new protection allowed for a single recursion, but should not have. After hours of trials and errors I figured out that Drupal might instantiate more than one view builder class, i.e. with separate $recursionKeys property. In order to avoid that, I've changed it to static one
There is behavior change in recursive rendering: the old method allowed up to 20 levels of recursion, while the new one does not allow a single one. It would be super weird if anyone has relied on that side effect.
In the end: the new approach correctly detects recursion and does not allow for it, while allowing rendering of the same entity multiple times and other similar things, or the same entity in different languages/revisions etc
The root cause for the issue is being fixed here 🐛 Use TrustedRedirectResponse Error on Multilingual Setup Needs work , but it's a dead end atm
taran2l → changed the visibility of the branch la_eu-3536805-3536805-add-randomized-grouped-staging to hidden.
so, it does not support HEIF for processing, only for getting the image dimensions - and the code to do that is the same as avif:
case IMAGE_FILETYPE_AVIF:
result = php_handle_avif(stream);
break;
case IMAGE_FILETYPE_HEIF:
if (!php_stream_rewind(stream)) {
result = php_handle_avif(stream);
}
break;
and the reason for that is (I believe): both standards are basically use the same container format ... technically AVIF is an AV1 encoded image in HEIF container, while HEIC is an HEVC encoded image in HEIF container. Very confusing actually :D
The problem is that HEIF image can ... pretend not not be HEIF, and ... be an AVIF
See https://github.com/nokiatech/heif/issues/74 for details
Our test image is like example C034.heic:
0x0000001C // 28-bytes: size of 'ftyp' box.
'ftyp' // boxtype
'mif1' // major_brand
0x00000000 // minor_version
'mif1' // compatible_brands[0]
'avid' // compatible_brands[1]
'miaf' // compatible_brands[2]
so it has mif1 and avif, but no hei* ... but PHP detects mif1 and decide that tis is an heir image which it's not true
I think PHP has broken AVIF detection by introducing HEIF/HEIC support here https://github.com/php/php-src/commit/dfac2da7fba930570fcda99205cfcf53db...
This affects multilingual sites only with a domain per language setup (and not a path prefix)
This has not been fixed!
taran2l → changed the visibility of the branch 3540531-test-on-php-85 to hidden.
So, the GD issue with rotate was indeed related to the default interpolation method change (should core now keep the BC and set it explicitly?)
The AVIF issues is mystery, as it fails on loading the image rather than processing. Is AVIF part of the docker image?
And due to new deprecations (PDO constants) most of the tests are red
taran2l → changed the visibility of the branch 3540531-fix-toolkitgdtest-for to active.
taran2l → changed the visibility of the branch 3540531-fix-toolkitgdtest-for to hidden.
So, it seems that the default interpolation algorithm has been changed, see https://github.com/php/php-src/pull/17375/files#diff-a6d1ba8e66261eacec5...
That probably means that we need to adjust test to that
Seems like rotation is affected either by our code or by how PHP8.5 handles it, as all rotate to 5 degrees tests are broken (file format is not relevant, it happens for all of them)
See https://git.drupalcode.org/issue/drupal-3540531/-/jobs/6225263#L2188
The other part is AVIF, I assume Docker image does not have AVIF support at all (or it's broken in PHP itself)
So, my idea was to recreate the file itself (maybe it has encoding issues), but that didn't work ...
hi @chizh273, there are more changes in the Drupal.nl private repo commit related to this issue, please transfer those over, see https://github.com/lembergsolutions/drupal.nl/commit/b1f22377d7718d98853...
that one looks good to me
addressed feedback
taran2l → made their first commit to this issue’s fork.
Per great suggestion by @danielveza, I have converted the test to Functional
Hope this can go in finally
taran2l → created an issue. See original summary → .
@cmlara thanks for acting on this so fast!
As I mentioned I've went ahead and changed scope a bit.
- I've simplified version constraints
- Enabled testing with previous major version (so 10.x is being tested)
- Add support for PHPStan 2.0 (which generates a lot of issues, probably a separate issue needed to resolve or ignore those)
- 11.2 support should can be handled separately, maybe here 📌 Copy core 11.1.x ExtensionMimeTypeGuesser to s3fs Active
PS I would suggest dropping support of D8 and D9 altogether, as well as PHP7 .. it's not being tested anyway, so is it still working ?
Limiting the scope to 10.5 per @cmlara suggestion
Implemented @luke.leber suggestion, please review. This should be ready to go in now
@catch, in #80 @tim.plunkett has self-assigned this issue and no progress for over a year ... is anyone has a better idea on how to overcome this issue without dependency on the route system?
Per @smustgrave suggestion I've made messages translatable, but this causes unexpected fatal errors in a few tests (I assume sometimes string_translation service is not ready yet/not available, but redirect already happens)
So, I've reverted those changes, so messages are as they are (i.e. not translatable)
However, I've one concern: UrlHelper is a simple class that does not depend on the container at all, and we are adding hook invocation to it.
A few tests are still failing, will take a look later
Addressed feedback by @smustgrave
Converted patch from #3098245-8: Clone nested inline blocks when translating → into MR + updated it after ✨ Fails on translated inline blocks Closed: duplicate got merged.
Went ahead and fixed this .... still supporting 9.5, it's up to you if you want to go like that
thanks for the fast turnaround!
Does not work actually
Blocked by 🐛 Error uploading svg from the media tab Active