Ok, looks like there is many discussions in core about this behavior:
ericgsmith → created an issue.
I had previously mentioned this in https://www.drupal.org/project/drupal/issues/3035352#comment-15214406 📌 Deprecate file_get_file_references(). Move the logic to file.usage service Needs work but I should have opened a new issue for this.
Copying the test and patch from that comment - it was very work in progress just copying over so its not lost - I suspect it still has a few issues but I've been using it for a while.
I recall I may have discussed this offline with somebody at some point (sorry, memory is terrible) and the idea was #3035352 would be a good place to solve this as it was being refactored into a service there - but given that has been postponed for a long time now hopefully we can git this fixed.
ericgsmith → made their first commit to this issue’s fork.
ericgsmith → created an issue. See original summary → .
+1 for this
@aman is MR8 ready for review? I see MR is open but issue still set to needs work - looks good and most importantly is working from an initial test - ping me if you want a review
Code change is good - would require declared core version in info file to be bumped to 10.2 and above
Looks to be working fine on D11.
DeprecationHelper was introduced in 10.1 and this module currently declares >=9 - this should be bumped to above 10.1 with this change.
++ please lets get this important fix in - is slowing down progress of 📌 Drupal 10.5/11.2 compatability Active
Adding 🐛 The namespace of EmbedCKEditor5PluginBase does not respect PSR4 Active as a related issue - that will need to land first for the tests to go green.
Ok - did end up trying to investigate these.
The last CI run shows the following tests failing:
58.307s Drupal\Tests\entity_embed\FunctionalJavascript\EntityEmbedDialogTest 0 passed, 1 failed, 1 log(s)
209.891s Drupal\Tests\entity_embed\FunctionalJavascript\ButtonAdminTest 3 passed, 1 failed, 1 log(s)
177.661s …upal\Tests\entity_embed\FunctionalJavascript\CKEditor5IntegrationTest 0 passed, 2 failed, 16 errored, 1 log(s)I ran these locally on 11.2.2 with same phpunit version. The first 2 pass (with deprecations):
$ phpunit -c app/core/phpunit.xml app/modules/contrib/entity_embed/tests/src/FunctionalJavascript/EntityEmbedDialogTest.php
PHPUnit 11.5.27 by Sebastian Bergmann and contributors.
Runtime: PHP 8.3.23
Configuration: /data/app/core/phpunit.xml
. 1 / 1 (100%)
Time: 00:38.220, Memory: 12.00 MB
OK, but there were issues!
Tests: 1, Assertions: 6, PHPUnit Deprecations: 5.
$ phpunit -c app/core/phpunit.xml app/modules/contrib/entity_embed/tests/src/FunctionalJavascript/ButtonAdminTest.php
PHPUnit 11.5.27 by Sebastian Bergmann and contributors.
Runtime: PHP 8.3.23
Configuration: /data/app/core/phpunit.xml
.... 4 / 4 (100%)
Time: 03:09.833, Memory: 12.00 MB
OK, but there were issues!
Tests: 4, Assertions: 32, PHPUnit Deprecations: 16.
The last one CKEditor5IntegrationTest.php does indeed fail multiple times.
RuntimeException: The autoloader expected class "Drupal\embed\Plugin\CKEditor5Plugin\EmbedCKEditor5PluginBase" to be defined in file "/data/app/modules/contrib/embed/src/Plugin/CKEditor5Plugin/EmbedCKEditor5PluginBase.php". The file was found but the class was not in it, the class name or namespace probably has a typo.
Looks like in the embed module that class is using the wrong namespace - it has Drupal\entity_embed\Plugin\CKEditor5Plugin
Thanks @smustgrave!
Confirming your change in d130db37 looks correct as per this change - https://github.com/ckeditor/ckeditor5/commit/4a595753940ab2087db7901922b...
Still have some unexplained test failures here - not really sure I'll have time to dig further into these
From what I can see from the original report was at the time 8.x-1.0-beta3
And the line in question was: https://git.drupalcode.org/project/cloudflare/-/blob/8.x-1.0-beta3/src/C...
So for that code to be executed - it looks like the client IP is already the connecting IP provided by cloudflare - so something is already restoring the client ip for you?
Still, that shouldn't blow up the site. The URL doesn't need to be in the log message, probably not appropriate to be doing that in the middleware.
Is anybody able to provide an update stacktrace to verify that it is the same issue?
https://www.drupal.org/project/http_response_headers → could also be used to help debug to see what headers you are getting on the request.
ericgsmith → created an issue.
Tests are still failing and it's not clear it it's related to this change or not, would be good to get a green pipeline.
Phpunit on previous minor is expected to fail as it's not compatible, but the current job is now testing on 11.2 and there is 2 failures there.
Re #16 - yes I didn't realise this wasn't already committed. It wasn't present but also isn't it the gitignore so not clear here. It's pretty common to commit the yarn.lock file as it can ensure dependency versions aren't changed until it's intentional
Made the following changes
- Bumped the Ckeditor version requirement in package.json to match the version of core 10.5.x / 11.2.x
- Bumped the Drupal core version requirement in the info file to ^10.5 / ^11.2
- Import from the publicly exposed constants instead of the source files
Sorry for knocking it back to needs review from RTBC - have tested on a project and is still working for me
ericgsmith → created an issue.
Ta, end of day for me here so I'll test it tomorrow
Change works and fixes the bug - just a minor in that most of the plugins I've seen have moved to using what is exposed by the icon plugin rather than directly referencing the icon path. Its a minor, but it could potentially save effort in the future if the icons are moved around within the plugin.
Yeah, unrelated to this MR but as I was changing the info file I saw and removed it on instinct - it would be good to remove it separately.
I'll rebase this MR and drop that commit.
Re patch - you can also reference just the commit that has the compiled plugin instead of the whole MR - https://git.drupalcode.org/issue/edit_media_modal-3531671/-/commit/12a9b...
Looking at
🐛
Could not find the fields "Path to java executable" and "Path to Tika .jar file"
Active
I've updated the search_api_attachments_update_8004 so that it only modified the config if needed, and doesn't touch the cache backend value set by the previous update hook.
It will be too late for sites who have already updated, but some warning text on the 10.0.4 release notes could be a good idea.
ericgsmith → changed the visibility of the branch 3533059-update-hook-changes-previously-set-value to active.
ericgsmith → changed the visibility of the branch 3533059-update-hook-changes-previously-set-value to hidden.
ericgsmith → created an issue. See original summary → .
ericgsmith → created an issue.
If somebody can review / test so that it can be moved to RTBC, I'm happy to reach out to maintainers to ask them to get it committed - but I think its fair to wait till the status is RTBC before doing this.
Thanks @drasgardian - that very good information to know, thank you for creating the relating issues.
From what I can see from your related is this code / change for Linkit itself is then ready to review as the issue is the responsibility of the other module?
I that case, I think I can move this back to needs review.
Have rebased against the latest 7.x changes
Looks good, still working.
Tested with dropzone and media_bulk_upload_dropzonejs disabled and the upload form works as expected, critical bug is resolved.
Tested with dropzone and media_bulk_upload_dropzonejs enabled and existing dropzone behaviour is still working as expected.
Maintainers - please lets get this committed. It is a critical bug that has existing for 2 years that several people have confirmed @godotislate's work fixes.
Please trust the community here and lets get this in. Thank you 🙏
Leaving for needs review for approach - but likely needs to go back to postponed until there's a released version so we can properly test with the xls serialization module changes.
✨ Any plan to integrate Box/spout ? Active was committed to XLS serialization module so I think this is ready for review.
Perhaps we should be checking that the sub module xls_serialization_open_spout is enable rather than just the open spout library exists 🤔
Test MR and all looks good.
Page loads fine before running update hook so form changes are working 👍
Update hook runs as expected and config after export has been updated with the expected values 👍
Looks good to me! Thank you
ericgsmith → created an issue.
Tests failing - current major still set to 11.1?
Fixed - pretty sure I've got the right link icon. It was removed from the way this plugin was loading it (see https://github.com/ckeditor/ckeditor5/issues/17304) and I saw there was a `IconLink` in the main package now so using that.
Core version req - I bumped this because of the plugin, but I guess some people might be using the filter without CKEditor?
Not 100% we need to bump it, if we don't then people on 10.4 could update before they go to 10.5 or 11.2 and it would break for them.
Quick test of the syntax I used>
drush php:eval "var_dump(\Composer\Semver\Semver::satisfies('10.4.7', '>=10.5 <11.0 || ^11.2'))"
bool(false)
drush php:eval "var_dump(\Composer\Semver\Semver::satisfies('10.5.0', '>=10.5 <11.0 || ^11.2'))"
bool(true)
drush php:eval "var_dump(\Composer\Semver\Semver::satisfies('10.6.0', '>=10.5 <11.0 || ^11.2'))"
bool(true)
drush php:eval "var_dump(\Composer\Semver\Semver::satisfies('11.0.0', '>=10.5 <11.0 || ^11.2'))"
bool(false)
drush php:eval "var_dump(\Composer\Semver\Semver::satisfies('11.1.0', '>=10.5 <11.0 || ^11.2'))"
bool(false)
drush php:eval "var_dump(\Composer\Semver\Semver::satisfies('11.2.0', '>=10.5 <11.0 || ^11.2'))"
bool(true)Duplicate of 📌 Drupal 10.5/11.2 compatability Active
Alright, got that pesky button working.
Bit of a shame the core version requirement has to be bumped - maybe there is a clever way to do this as a shim to keep support for both. Either way, this should be working now.
Seems one potential fix here would be for the server backend plugin to include empty fields in their result items if they tried to retrieve the field, since this means that the item was indexed without a value for that field and explicitly returning the field as empty makes sense.
Sounds related to this Solr issue which is proposing to do this ✨ Empty field leads to full entity load Needs work
Easier to show what I mean with an MR - setting to needs review.
If this idea is something that could be accepted I can work on a patch - otherwise we will likely implement this by extending the processor to add the query tag check in a custom module - but we prefer to avoid custom code and upstream the solution if we can.
ericgsmith → created an issue.
Plugin change looks good - but I think this will also need a change to core_version_requirement in the info file to be 10.5 and later for 10 and 11.2 or later for 11
ericgsmith → created an issue.
ericgsmith → created an issue.
Also noting that while the const is named MAX_TAG_PURGES_PER_REQUEST this is used to chunk all invalidation (e.g urls too) which do have the same limit (https://developers.cloudflare.com/cache/how-to/purge-cache/#single-file-...) but perhaps we can make this clearer by renaming it.
Test / lint job failures unrelated
Unfortunately tests and a lot of linting jobs failing on 2.0.x branch - so the pipeline looks mess, but I've moved the config from cloudflare to cloudflarepurger module given there was a specific form added in 🐛 Excessive Tag Hash Collisions Fixed for the sub module config.
The only part I don't like is the purger needing both the cloudflare.settings and cloudflarepurger.settings configs but 🤷
This also now has the benefit the the constant for the cache tags per request can be directly referenced - which also needs to be bumped in 📌 Increase cache tags per request to 100 Active .
ericgsmith → created an issue.
From the docs:
If the bucket is empty, further requests must wait until new tokens are added. This system maintains fair usage while allowing occasional bursts within the bucket's capacity.
So you are responsible for retrying the last request - Cloudflare is not queuing them.
Their blog had a good example:
For example, a free user starts with a bucket size of 25 requests and a refill rate of 5 requests per minute (one request per 12 seconds). If the user were to send 26 requests all at once, the first 25 would be processed, but the last request would be rate limited. They would need to wait 12 seconds and retry their last request for it to succeed.
Each of those requests being a maximum of 100 tags (which I will open a separate issue for since the module currently limits to 30).
The limits are so varied too between plans - that I think it will be pretty complicated to implement a check for this. That said - its not a limit on number of cache tags anymore - so I think DailyTagPurgeLimitCheck can be removed completely.
Its then a question of if the module needs to implement a BucketSizeCheck of some kind.
Something along the line of a BucketSize counter which
- starts at the default size (e.g. 25)
- decreases by 1 for each request
- increases by 1 every X seconds until it hits the default size again
I would suggest opening an issue to take over the maintenance of the project → if somebody has capacity for this.
As noted in his bio:
I am retiring a bit from Drupal.org, so unless one of my projects is actually broken I will probably not be checking on the issue queues. Thanks!
More context for this is also available here - https://www.thedroptimes.com/interview/48921/drupal-core-static-site-inn...
Thanks for picking up the work here, it is awesome to see all the issues closing and to have a stable release!
Do you have permission to opt into coverage of Drupal’s security advisory policy? Now that this module has a stable release it would be great to have it covered by the policy as well.
Hi team,
Fantastic news to share - https://blog.cloudflare.com/instant-purge-for-all/
Cloudflare now supports tag invalidation for all account levels - if you are using Cloudflare I suggest trying cache tag invalidation so that you can move away from this module.
Ready for review.
Module was broken on 11.x after the NodeForm class has moved namespace: Drupal\node\NodeForm is now Drupal\node\Form\NodeForm →
Change record: https://www.drupal.org/node/3517871 →
The CR doesn't seem to imply we should need to do anything for BC as the issue references the use of the deprecation class loader. However - autoloading does not apply when we are type checking - i.e when we do instanceof it fails. And not fails like throws an error, it fails in that "$form_object instanceof NodeForm" evaluates to FALSE (as the class doesn't exist) and silently moves on.
Calling class_exists will trigger the autoloader and return to normal functionality (i.e allow this to work with 10.x and 11.x) but seeing as this is explicitly marked as an @internal class we shouldn't be using it at all. Given all the login was within an if statement that checked it was a node form, I've just changed to use the node form base form alter hook.
Thanks Adam, and thank you for all the work here!
ericgsmith → created an issue.
Changes on the MR11100 look good to me and likewise with #135 I have tested this and the functionality and upgrade path work as expected.
I've been running versions of this patch in production for a while now without issue.
I think this is ready to go to RTBC - the only thing missing was a change record. I have added one https://www.drupal.org/node/3529709 →
If somebody can please review / amend the CR then I think this should move to RTBC
Agree with this.
Also tested the original issue of 📌 Change the Webform libraries CDN warning to an error in the Status report (/admin/reports/status). Fixed and had no issue when big_pipe was enabled and a webform using input mask from CDN. A warning is definitely more appropriate here.
Restoring status from #41 as MR still applies cleanly, bot appears to be wrong.
Patch applies cleanly to 2.0.x still - leaving as needs work as we still need a test although previous comment is now outdated as we have gitlab ci setup
+1 for RTBC, test fixes / changes look good
Commit applied cleanly and tests on MR all green
I'm reopening this as this was comitted to 10.4.x but not 10.5.x - which doesn't feel right? I'll open an MR with the commit on 10.5.x
ericgsmith → made their first commit to this issue’s fork.
Have tried to see where this is at.
The comment in #99 links to the 10.5.x pipeline which we can see is broken - as far as I can tell the 10.4.x pipeline from the same commit is https://git.drupalcode.org/project/drupal/-/pipelines/358805 which did fail but looks like an unrelated failure.
I've reapplied the commit from 11.x to the 10.4.x and it applies cleanly and appears to still pass https://git.drupalcode.org/project/drupal/-/merge_requests/12272/pipelines
I've done the same for 10.5.x here and its failing with what was reported before it was reverted https://git.drupalcode.org/issue/drupal-3278759/-/jobs/5409866#L412
The reason appears to be what was outlined here in #91 - except that the related issue ✨ Improve Dynamic Page Cache header assertions in JSON:API tests Needs review was committed to 10.4.x but does not appear to be on 10.5.x. That issue was closed as fixed - but my assumption here (have not tested) is that if that issue is backported to 10.5.x then this should pass.
I'm a bit confused on the timeline of when branches were open - but should this be postposed on ✨ Improve Dynamic Page Cache header assertions in JSON:API tests Needs review - and should ✨ Improve Dynamic Page Cache header assertions in JSON:API tests Needs review be reopened so that it can be applied to 10.5.x? Seems very unexpected that we have something in 10.4.x but not in 10.5.x?
ericgsmith → changed the visibility of the branch 3278759-10.4.x to hidden.
ericgsmith → changed the visibility of the branch 3278759-10.4.x to active.
I've added test coverage.
https://git.drupalcode.org/issue/easy_breadcrumb-3305870/-/jobs/5365762#... shows the error I mentioned in the MR:
There was 1 error:
1) Drupal\Tests\easy_breadcrumb\Functional\EasyBreadcrumbLayoutBuilderTest::testLayoutBuilderSupport
Exception: Warning: Undefined array key "entity_type_id"After reverting fdf560dd the test now passes : https://git.drupalcode.org/issue/easy_breadcrumb-3305870/-/jobs/5365770
However this reintroduces the phpstan issue.
Have not done a proper review of the code / changes - likely the intent of fdf560dd should still be progress to resolve the issue although now there is test coverage to ensure it works.
Setting to needs work as it either needs the baseline updated / accepted or improvements to not introduce new phpstan issues.
ericgsmith → made their first commit to this issue’s fork.
On further investigation - I don't think views is smart enough to add all the specific contexts as the issue summary describes, I can only see it adding the full url context here https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/views...
I.e. if a page contains a view with exposed filters, each filter's machine name will be present in response cache contexts list as `url.query_args:`
Are we sure this premise is true? I'm just trying this patch on an existing project and don't see this.
I've got a view with exposed filters, which exposes a taxonomy term with the url query param "type". When I apply this patch and debug the cache contexts I see the more generic url.query_args context is present but nothing specific for type - there is no url.query_args:type. I don't know if that is optimized away at some point or if views isn't adding it - I have not debugged further. With the current patch its then leaves type in the $query_params and the returns NULL, incorrectly skipping this page from the registry.
I need to test this on a clean install / check where cache contexts get optimized as query param garbage is a huge problem for me too / this module and am super interested in solutions to this problem.
ericgsmith → made their first commit to this issue’s fork.
ericgsmith → made their first commit to this issue’s fork.
+1 for RTBC - this is super important to get in and a major accessibility issue
Have closed 🐛 "List of links" not working in 3.0 Active as a duplicate. This is technically a duplicate of 🐛 Links widget is loading the checkbox widget JS library Needs work which is older, although the solution here is complete and RTBC'd already.
Suggest the maintainer consider closing that and assigning credit from the related issues.
MR was showing with an unreleated test failing - have rebased and showing as green now.
Adding 🐛 Links widget uses checkbox widget library Active as a related issue which I believe solves this and takes the patch here further to do additional cleanup.
Not sure what the etiquette is here for closing duplicates as this issue is older - but that issue has a more active MR? Either way - #3447859 is RTBC so I think focus should be there.
ericgsmith → made their first commit to this issue’s fork.
Is this a duplicate of 🐛 Links widget uses checkbox widget library Active - that issue seems to go further than just reverting the class change.
Encountered the same as #9 - error shows up when rebuilding the cache after updating from v2 (I have verified that the deployment identifier changed) but does not show up if updatedb is run before clearing the cache.
The first time the cache is rebuilt I saw an error:
$ drush cr
In EntityTypeManager.php line 142:
The "group_relationship_type" entity type does not exist. The second time I see this error:
$ drush cr
In FieldConfig.php line 124:
Attempt to create a field without a field_name. As #9 mentions - it looks like drush deploy does not clear the cache before running update db. Our deployment steps do - which can be changed, but curious if this could be hiding an issue. Looking at the issue for the deploy command it looks like it was removed as it wasn't necessary as updatedb runs with a null back backend, and there is no instruction to do it in the drupal update docs → either - but from a quick google I can see we probably aren't the only ones doing it - e.g Lullabot.
So perhaps my issue is a deployment order issue given the steps to reproduce are completely different to the issue and other's comments such as #10 - but it might also be highlighting something wrong here.
Moved patch to MR - CI shows test job passing + expected failure of the test only change
Moved patch to MR
Ci shows test passing + test only job failing to demonstrate the error.
Lint jobs - lots of errors / warnings on 3.0.x branch for most jobs - does not appear from a quick look to be introducing anything new here.