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
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.