I believe this is good to go - I will wait a few days to get feedback on update hook #9 otherwise I can merge and tag a new release next week.
Have made an adjustment to the gitlab CI file so that cspell is green and its testing against 10 and 11.
@gaurav.kapoor I believe we don't need an update hook here. Existing config would have been taken care of by views_post_update_remove_default_argument_skip_url - I have tried and install on 11 at 71ead06d2e39495e70de46b94d99f62d6e8254fd (before the commit to tidy up the config) and when exporting the config I do not see the default_argument_skip_url - presumably as when installing its not part of the schema so is ignored.
ericgsmith → made their first commit to this issue’s fork.
ericgsmith → changed the visibility of the branch 3169876-better-handling-of to hidden.
@dqd thank you for the getting the ball rolling on this one 🙌
I've made an assumption here the needs work related to getting the CI jobs green.
I've made some minor changes to get cspell and phpcs happy.
I dropped the drush requirement from the composer file - this was preventing CI from installing - I don't think this should be specified by the module as there does not appear to be any drush integration.
I added a basic test using the steps described on the project description to validate that #5 is not an issue. This could probably be optimised to be a kernel test - but given its the only test in the module and the code doesn't change frequently I'm not too concerned about the speed.
I think this is ready for review now
ericgsmith → made their first commit to this issue’s fork.
Second thoughts, this is small enough that we should have test coverage - will include gitlab ci file and see if we can get green tests here
Bumped version number.
Tested in 11.x with no issues - evidence in https://www.drupal.org/files/issues/2025-11-09/Recording%202025-11-10%20... →
- Applied gitlab ci template
- Fixed eslint issues
- Fixed the automated phpcs issues, this job still fails with a warning - could potentially solve these here but its fairly long manual review of all the comments and functionality to fix, I'm not comitting to doing this at this point
- Fixed 1 phpstan issue and comitted the baseline to ignore the 2 di rules - again could be done in a follow up if needed
Opened MR with the change and tested on a standard install of Drupal 11.x
No issue found when configuring download options and field formatter.
No issue found viewing and downloading the files.
There is no Gitlab CI setup for this project or tests - I will open a separate issue for this as it would be good to prove the compatibility with a test.
Final comment - I appears that it is still reproducible in 7.x if you do an additional step to remove the displayed text field value before clicking Insert - so something is still up there.
I am able to reproduce this behaviour on the latest release 7.0.10 but I am not able to reproduce it on the 7.x branch.
There is definitely a bug here - but potentially ✨ Display node title (a text) in by default when creating link in ckeditor5 Closed: duplicate either resolves or masks the issue.
Testing on 7.0.10 - shows the issue but also note that the display text is blank (e.g 3388565 is not in play)
Testing on 7.x - shows the issue is no longer present, and shows 3388565 has been applied
https://git.drupalcode.org/project/linkit/-/commits/7.0.10?ref_type=tags
The commit 4a6ca22dc18beb8d232ae388c490db53e41295cc does not appear to be associated to any issue.
✨ Display node title (a text) in by default when creating link in ckeditor5 Closed: duplicate is not part of 7.0.10.
In fact, completely unintentionally I think this issue resolves whatever the bug in 7.10.0 is. I'll move this discussion to your issue
This issue was committed after 7.10.0 - it is not part of a release? https://git.drupalcode.org/project/linkit/-/commits/7.x
Hi @mark_fullmer - thank you for comitting - massively sorry about this but I did break backwards incompatibility for older CKEditor versions. - they get an error with object property undefined which breaks the select handler completely.
Have pushed a fix for this to https://git.drupalcode.org/project/linkit/-/merge_requests/145/diffs
Love to see this - not only is the UI better but the change is now super simple - can remove all the changes to the attributes that we first tried.
Back for review - example here showing the new behaviour when no text has been entered, and existing behaviour to keep text if already entered:
As noted in #23 the 700 character limit is not enough to deny everything and I think not practical for a comprehensive policy. It also doesn't seem to be based on anything.
In practicality http header sizes are restricted by server implementations - given this is essentially supercedes the feature policy, is there a reason not to just use a textarea like feature_policy_policy is using?
Had a quick look when implementing a policy for a client.
Missing standardized features
- attribution-reporting
- compute-pressure
- direct-sockets
- storage-access
- window-management
Missing proposed features
- autofill
- deferred-fetch
- language-detector
- language-model
- manual-text
- rewriter
- summarizer
- translator
- writer
Missing experimental features
- all-screens-capture
- captured-surface-control
- digital-credentials-create
- digital-credentials-get
- monetization
- smart-card
Missing retired features
- window-placement (currently grouped as experimental by module)
This was just a manual side by side comparison - potential that I've missed something.
Looks like a bit of work still needed here on tests and cleanup
@mark_fullmer thanks Mark, I missed that change when rebasing.
Should be a simple enough change to set the value of "Displayed text" if its empty instead of waiting for the link to be inserted - I'll hopefully have some time to take a closer look soon.
Targeting 2.2.x and moving patch to MR - will see if the tests are still passing
ericgsmith → made their first commit to this issue’s fork.
Rebased against latest changes from 7.x - saw in 🐛 Cannot replace 'Displayed text' of existing links Active changes from null to empty string so applied that to the new attribute we introduce here.
Since I have rebased this multiple times are we have lots of commits I've moved the build js to a separate commit. I think this makes it easier to review the commits and also will make it easier to resolve merge conflicts in future as we just need to build the assets once vs for each commit.
@justcaldwell @rajab natshah just checking, did either of you open an issue for Core regarding your comment in #8?
I think given core is already shipping the bookmark plugin it would make sense for the future of both modules if core supported the Drupal integration of the plugin?
Added a test to show the issue.
Work in MR!13255 is mostly hacking around - I'm not really sure this is the responsibility of menu ui to know about content moderation, but then there are already existing hacks there doing the latest revision dance / changes.
The changes in 🐛 Disallow saving the current default revision as a non-default revision Active now make this situation a lot worse for workflows that do not allow a published => published transition.
Now we can see a forward/pending revision on the menu item - and when published via the content moderation form that menu revision becomes stuck.
Menu manipulation is then blocked as you get the error message: "Manipulation of a menu tree having links with pending revisions is not supported, but you can re-enable manipulation by getting each menu link to a published state."
But its not clear if you can ever get the menu item into the correct state.
I will test this on a clean 11.x install soon and update the IS title / steps to reproduce.
@stewest looks like we've got some namespace collision - this module created originally with sector 9, it looks like no relation to the module which has since been added in sector itself - moving to sector issue queue :)
Magic! Thank you @dhruv.mittal hopefully maintainers agree and can commit this. Changes are good and resolves the issue when using this module with distributions.
Re #37 - tests are also failing on 2.0.x branch for the previous minor which is not affected by the CKeditor version change - I'm not sure how much effort is due here vs a separate issue.
Previous minor tests on this branch - well we kind of expect them to fail right, the JS is not dependent on a CKEditor version only available in 10.5 and 11.2 - given 10.6 and 11.3 will be out at some point, it makes sense to keep the tests running on prev minors? Maybe one for the maintainers to give input here.
Other tests - I'm not convinced anything is broken. I already explained in #27 what I investigated - I added a new branch with the patch committed to #3517882 which was responsible for some of the failures. We can see here that the tests are passing https://git.drupalcode.org/issue/entity_embed-3531672/-/jobs/6389586 except for CKEditor5Integration which gives:
"Test was run in child process and ended unexpectedly"
The tests pass for me locally - if somebody wants to do battle with gitlab to figure out what / why the pipeline is crashing go for it - but I think I still have confidence the test failure is not related to this change and that the work everybody has done here is good.
ericgsmith → changed the visibility of the branch 3531672--drupal-10.511.2-test-with-embed-fix to hidden.
ericgsmith → changed the visibility of the branch 3531672--drupal-10.511.2-test-with-embed-fix to hidden.
Thanks Thomas - the update to the README looks good to me - thank you for the review!
Nice one have done, thanks for checking Dieuwe
I'm happy to tag + release if this is all good - just wanted to check in case there was an other reason why dev was added
ericgsmith → created an issue.
Nice one, change looks good and fixes the issue 👍
https://git.drupalcode.org/project/cloudflare/-/jobs/6238954 shows the issue well - looks good to go!
Updating issue summary and title to clarify the impact here.
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