🇳🇿New Zealand @ericgsmith

I have given this another round of improvements - most conversation points are on the MR.

I appreciate you were part way through a review @Megachriz but I hope what I have done has simplified it so there is less to review.

Summary of recent changes

• Reduce UI complexity by offering only one new option "Only process mappings found in the source"
• Test for the FileTarget to show that the issue reported in comment #18 is resolved.
• Opened ✨ Avoid setting a value to an item when source is missing Postponed in feeds tamper for comment #19

I have also updated the issue summary with what is proposed.

I think approach looks good - added a comment to the MR as its using incorrect column definition.

Added initial workaround.

ericgsmith → created an issue.

ericgsmith → created an issue.

Looks good to me - we (note - I am a colleague of xurizaemon and amanp) have been using this patch on a site with postgres for several years.

Looks good and works

Added an event subscriber based on the system files download route instead of using hook access.

May not work for systems where files can be downloaded via another route (not sure this would be common?)

We rely on several patches to work with this module, so tried to not move the code / currently logic for tracking the download - only renamed the method so that other patches still applied cleanly but can be refactored if any of these issues are committed.

Have done some more testing with CSVs and its working pretty well.

I have one remaining issue and that is with fields with that have tampers applied to them.

When feeds tamper runs for a missing source it passes NULL to the tamper plugins.

If it either receives NULL back from the tamper plugins on L126 or if we have the SkipOnEmpty plugin causing NULL to be set on L131 here then the item suddenly have a NULL source value an sets this to the field.

This is more complicated to solve, as I have things like the default value plugin to provide values for things that are always missing, but also have had to use SkipOnEmpty as lot due to working around issues summarised in ✨ Improve handling of empty data Needs review

I'm still keen for a review on this initial implementation so leaving as needs review, but will need to also think about how this affects feeds tamper.

Also have a customer reporting this issue.

Occurs infrequently and is always resolved clearing cache. Have not been able to produce it locally.

Found an issue with the partially incomplete source values with file / image fields. The target_id set when attempting to merge with current values later caused issues if the reference_by was not the fid.

I changed the approach slightly to get the target plugin to return the current target values for this situation - although its starting to get a but messy.

Nice work @klidifia!

I am reopening this issue since its hidden being closed when there is work continuing on it.

I agree that it sounds like it is not exactly a duplicate of ✨ Show entity title after autocomplete selection instead of internal route (e.g., /node/123) Needs work although that issue would be useful alongside this one - but that is specifically talking about the UI shown after making a selection from the autocomplete - where as this is about the default text content of the link.

I have this a test on a clean install and it looks good to me - updated issue summary with a gif recording of it.

I think the existing LinkitDialogCKEditor5Test should be updated to show that the default text is the entity label. Setting to needs work for this change.

I also did not get the error reported in #11 - but I see in the js log it mentions editorAdvancedLink - I have not tested in combination with this module but could that be doing anything to interfere with the attributes?

Ok - I have enough there for an early review.

I have only tested and focused on this using CSV as a source - I imagine it could be more complicated with other sources depending on how they provide their data - specifically if they are provide NULL for empty values rather than missing values.

This MR introduces a new option under the Advanced tab:

The wording is not particularly friendly - but I wanted to distinguish between a field mapping having all source values provided and some source values provided.

E.g - if one of body:text or body:summary is missing - is it clear that we expect the body field to be skipped, or that we only want the provided source updated (e.g. only update summary, leave text in place).

Additional to the tests I have given this a go on a site with many fields mapped in a feed - and it works as expected. A few cool things this unlocks alongside skipping fields when they are completely missing:

• updating alt text via feeds without having to have the file id in the source
• updating body summary values without having to have the body value in the feed

I think if the general approach can be reviewed - the follow ups would be testing with other parsers and then improving the UX for the field labels and descriptions.

What about using "Source" and "Target"?

I've been using Gitlabs compare revisions a lot - and I know the context and UI is different, but I feel like maybe the same label be adapted?

E.g left column = "Source", right column = "Target"

"Source" and "Target" then with something to explain it in the help text link:

"When comparing selected revisions the changes are shown as if the source revision was being updated to the target revision."

I have a few clients who often use this screen, I can ask them to review options if we can get a few other ideas in the mix.

I have taken a look at this today (coming from ✨ Update target more granularly Active )

Feeds cannot always make the distinction between empty vs non-existent. This was stated in #1107522-147: Framework for expected behavior when importing empty/blank values + text field fix.
We should test whether or not this is still true for the current version of Feeds or if this was the case for the D7 version of Feeds only.

I assumed my test for this would fail - as all I could see to check on the item was if it was NULL. I suspected that there would be a difference between NULL and empty columns. But for the CSV at least it is seems to parse ,, to an empty string - so we can for CSV at least tell the difference between an empty item and a missing item.

Mapping multiple times to the same target (a way to import multiple values for the same field).
If the first mapped source is non-existent, will the target still be emptied when the second mapped source does exist?

I have not tested this - but I believe yes, if source 1 => field_1 is missing from the source then the target field_1 is not cleared. If source 2 => field_1 has a value then target is cleared.

Expectations of the current behavior.
Right now it is ensured that all mapped targets will get overwritten. So I think we should at least keep supporting that workflow, in order to not break existing import workflows that rely on that behavior.

Agreed - in my work in progress branch I have added this to the advanced fieldset if update existing is checked.

I also have an additional question around targets comprised of multiple sources.

Any feeling for how these should be handled?

An example I added to the test was mapping to both body:value and body:summary from a CSV field - I'd love to be able to update just the summary and keep the original value but its pretty unpredictable what the output would be here - partially missing / updating fields would likely mean we need further changes in the target itself.

ericgsmith → changed the visibility of the branch 3162496-feeds-update-existing to hidden.

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

ericgsmith → created an issue.

ericgsmith → created an issue.

Yes, sorry this has been a major pain point for many.

https://www.drupal.org/project/tamper/issues/3332785 ✨ Improve handling of empty data Needs review is where I tried a while ago to consolidate this but haven't had time to push it further.

Happy for this, we probably just need to update the unit test cases as well to add one for null and ''

The issue of duplicate items going into the queue is a purge issue https://www.drupal.org/project/purge/issues/2851893 ✨ Deduplicate Queued Items Active

I have found success with Jonathan's module https://www.drupal.org/project/purge_queues → mentioned in that thread.

For larger sites we still find issues with the performance as the duplicate check queries items individually.

I currently use this patch https://www.drupal.org/project/purge_queues/issues/3379798 ✨ Alternative approach to unique queue using upsert Needs review along with his module which has been good for a year or so.

The issue of actions like saving one node and its filling up the queue with all items is down to your site and cache tags. Have a look at the cache tags stored against the urls. Common culprits are things like node_list from views. Modules like https://www.drupal.org/project/views_custom_cache_tag → may help if that is the case.

As for a 2.2GB url table, that is huge! I'm sorry don't have much to offer, try analyze what is in there and if you can ignore any it through the blacklist. Things like facets and search terms can flood the table, if they are cached consider setting a lower lifetime and ignoring them. Cache invalidation for some of the site maybe be better than none.

This module also has an issue that pages never expire/ drop off the registry so crawlers and garbage urls can really pollute the table. https://www.drupal.org/project/purge_queuer_url/issues/3045503 ✨ Registry expiration as opposed to removing it too soon Needs work

I'm really sad to see that the owners of ckeditor_module decided to take the features that I did on this module and include them in the ckeditor_module without any notice

I don't think this was an active decision by the maintainers - this was just what the person who provided the original patch did.

Agreed, setting back to needs review.

Rebased the MR.

We are using this patch and editors found it an improvement of the diff screen - I note there hasn't been much discussion on this since #11 so not setting to RTBC, although I can say I have one client happy using this approach.

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

I don't think this module should necessarily uninstall the ui module, people may want the module enabled after upgrading to this modules in order to access / migrate their data.

That sounds more like something that should be in the release notes.

Needs a reroll for 2.1.x but wanted to query something first.

#10 still applies cleanly but does not work for date fields, only datetime fields. It does this by not modifying the service and passing in the start date for the end date.

#12 would need a reroll, but it aims to resolve this in DateRangeCompactFormatter by allowing null values rather than changing the default values. But even with formatTimestampRange modified to accept a null value, its keeping the login from #10 to set the end date as the start date.

I think given this is postponed, it would be simpler to keep the logic for #10 and apply it in the if block for date only fields?

I've tried to go with @joshuami suggestion of a new MR based on the patch, as I found an additional bug that I needed to fix for a client - but I wanted to keep some of the stuff that was only in the MR and not just the patch.

I don't want to confuse the issue further - but this is what I did:

• Applied the patch from MR4 to a new branch from 8.x-1.x (A)
• Applied the patch from #146 to a new branch from 8.x-1.x (B)
• Cherry picked from B into A to get the changes from the patch into A
• Added additional commits to B and pushed as mr4-with-patch-146

The merge request adds an upcast and downcast in /js/ckeditor5_plugins/ckeditor_templates/src/ckeditorTemplatesCommand.js that adds the widget type around handles to add a new line before or after the inserted template.

This is still missing from mr4-with-patch-146 - I'm still in favor of adding this back if its configurable.

As previously mentioned - this is a big beast, the last thing I wanted to do was add more confusion the the issue with another MR, but hopefully this is the consolidation between MR and patch that can push the issue forward. Setting to needs review - would appreciate if the approach to use this new branch is acceptable we can hide the patches and old branch.

Actually reviewing #11 - while ECA should not be converting it to an array there is still the same issue here.

base64_encode only accepts strings, but serialize and json_encode can handle multiple values.

Maybe we should split them all out?

Just wanted to sanity check the deprecation removal - https://drupal.slack.com/archives/C34CECZAL/p1714710131685999

As discussed above - there has only been an issue with ECA using this plugin, or with feed tamper if decode options were called with a multiple value - calling with decode and a single value via feeds tamper is working - so feeds would be impacted by removal. I'm happy with the change.

Not tests fail with this change - interesting!

Looks like DateTimeFieldTest::testDefaultValue only tests for date only fields which have not changed here - it will need some test coverage for datetime.

Converted patch to MR - curious to see what the tests will show.

I'm not sure the possible side effects of the change, but for me it doesn't make sense to use the storage timezone for relative dates in the UI - its confusing and limits some good meaning defaults we can have.

Found this issue after what seemed like a very simple request from a client "we want the default value to be the next day at 2pm" - can't do it as even doing hours relative to UTC is impacted by timezone changes.

A relative value of "tomorrow 2pm" works with this patch.

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

What is hook_file_transfer? I can't see it / find any docs for it - is that coming from a different contrib module?

Will need a similar solution to #2913616

ericgsmith → created an issue.

ericgsmith → created an issue.

ericgsmith → changed the visibility of the branch ckeditor_templates_ui_migration-3273358 to hidden.

ericgsmith → changed the visibility of the branch ckeditor_templates-3273358-3273358-ckeditor-5-support to hidden.

ericgsmith → changed the visibility of the branch 3273358-ck5 to hidden.

Thank you @joshuami for the detailed summary, that is very much appreciated for somebody who is trying to keep track of this monster 😅

This issue has faced many challenges due to dancing between patches and MRs - for big changes like this MRs are fantastic for keeping track of issues and feedback that gets lost along the way with patches. Unfortunately multiple attempts to move the patch to MR end up with patches reappearing without interdiffs, and then MR becoming stale. The MR has the latest commit, but misses 7 months of development between #91 to now. I am hesitant to keep trying to work in the MR if it just leads to more maintenance when people start uploading patches again but its really hard to keep track of every reported issue in these 150 comments.

I just want something committed to a dev branch so we can open up remaining niggles as follow up issues - make them easier to review and ensure we are not reintroducing issues already fixed.

What do people thing about:

1. Create a new 2.x branch or update the 8.x-2.x branch with the changes committed to 1.x
2. Create the MR as @joshuami suggests
3. Commit to the dev branch and be realistic about some features being added as new follow up issues

I think that will make it easier to drive this forward.

Things like the widget functionality - there are arguments both for and against in is thread, and a good suggestion to make it configurable. Why not merge what we have a keep that conversation going in a dedicated issue.

While my review above that this works well stands, I've noticed 🐛 Excessive Tag Hash Collisions RTBC introduced a new config schema and form for the cloudflarepurger sub module where as this is using the cloudflare settings.

I think the config here actually logically lives in the new sub module schema? Its a bit messy as the purger would then need to reply on both the cloudflare and cloudflarepurger config.

Tested patch #9 and it resolves this error.

Did not test MR but looks exactly the same - needs to be updated to target 2.1.x branch?

@arora.shivani - I took a look and the patch doesn't do anything for the errors reported specifically in this issue - I think my original comment stands that your issue is likely in 🐛 Unable to add an extra field on a content type/bundle that has no extra fields already Active

Added an update hook to correct any broken config.

This I think can be used for either case if the id change is reverted or not.

It will help people who have either created config using the changed id format - or if the id format is not going to change back it will convert old config to the new format.

The new format is I think a mistake, if you have the same bundle name across multiple entity types (rare but possible) this could lead to collision, I believe it was just a mistake that can be reverted.

There is a Characters to trim plugin that doesn't trim whitespace?!

The trim plugin only trims from either the begging or ending of a string. As the whitespace is in the middle, that is not the plugin to use for replacing characters in the middle of strings. It's just a wrapper around phps trim, ltrim and rtrim functions.

I can take a look at why find and replace didn't work. But glad you found a solution.

Hi arora.shivani,

There is a different issue reported in 🐛 Unable to add an extra field on a content type/bundle that has no extra fields already Active which is specific to your first issue.

There is already a patch in that issue that is the same as yours so it looks like that is another valid fix that needs to go in but this issue is about existing data not fresh installs.

Please do follow up in that related issue that the patch works for you.

Setting back to needs work based on my previous comment / MR.

Opened MR to revert the change which I believe is accidental.

This will still need an update hook I believe, as this will cause the same issue for people who have created config from 2.1.0-rc1 or when using the committed diff as a patch (as indicated in the related issue, some people are using the committed change already having updated the config ids themselves).

Setting needs work as I think with or without the id revert this will need an update hook to ensure what is in configuration matches what the entity generates for its id.

This is a regress from this issue 🐛 Unable to create extra field on user entity Fixed

That changes the ID of the config - but there is no update hook provided to fix existing config.

What is happening is that the the config storage loads all the configs which are keyed by the old id:

    foreach ($this->configFactory->loadMultiple($names) as $config) {
      $id = $config->get($this->idKey);
      $records[$id] = $this->overrideFree ? $config->getOriginal(NULL, FALSE) : $config->get();
      $configs[$id] = $config;
    }
    $entities = $this->mapFromStorageRecords($records);

When it loops through the entities returned, they have the key id, so this thows an exception as $configs[$id] isn't set - configs is keyed by the old id.

    foreach ($entities as $id => $entity) {
      $self_referring_cache_tag = ['config:' . $configs[$id]->getName()];

This module needs an update hook to fix the old configs.

Have been using this patch for a long time in production for multiple sites now without issue. Looks good and works a treat

ericgsmith → created an issue.

Added a new test method to the existing Image Style unit test to check that the registry and cache invalidation is not called if flush is called with a path. Checking the number of invocations seemed simpler than trying to dig through cache ids.

Hid the original MR - I see now according to https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... → I should have pinged you to update it the MR - anyway, no open discussions on the original so hopefully all good.

ericgsmith → changed the visibility of the branch 3439981-uploading-a-file to hidden.

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

Found this issue while investigating a performance issue on a site when using views bulk operations to bulk publish media.

Thank you @fago - 🐛 Uploading a file to media library flushes theme registry Active and this helped pinpoint some of what I was seeing 😱

Tested patch - code change looks good and fixes the issue.

The patch from 2779485 🐛 Ajax breaks password reset Postponed: needs info works to hide the error at least, but still leaves AJAX in place. I think 2779485 started off as a different issue however since comment 5 and 6 perhaps they are experiencing this issue.

ericgsmith → created an issue.

Pushed a 4.x backport to the fork - have a need for this patch on a site that is still on 4.x due to an older version of Sentry - hiding MR but there in case anybody is in a similar situation.

ericgsmith → changed the visibility of the branch 3408332-backport-4.x to hidden.

Thanks Mark - I've tested the port to branch 3408332-port with Drupal 10.3 and its working as expected for me

We have been using this patch in production since @RoSk0's improvements around 8 months ago.

We have not found any issues with it, and it has greatly improved the performance of the application when large amounts of invalidation occur such using bulk uploads or bulk editing content.

@Berdir Thanks - I have updated the test - it was failing on 10.1 as AjaxPageState was introduced in 10.2 not 10.1 as I previously noted incorrectly in my comment.

I think the bug is still relevant for 10.1 if other code is modifying request parameters - I've added a check to the test to see if AjaxPageState exists and to modify the request params in the test if it doesn't - so test runs and passes on both current and previous minor.

Have fixed the existing test and taken a closer look.

I think we need to be mindful here for pages using the internal page cache module.

The behaviour for page_cache is permanent by default:

    // The response passes all of the above checks, so cache it. Page cache
    // entries default to Cache::PERMANENT since they will be expired via cache
    // tags locally. Because of this, page cache ignores max age.

While some setups will have the internal page cache disabled and rely only on an external page cache, not all will.

If page cache is enabled, it looks like the url registry item is removed after expiration, but the as the page cache entry will remain and continue to be returned by Drupal this never gets added back to the registry because of:

    // When page_cache is enabled, skip HITs to prevent running code twice.
    if ($cached = $response->headers->get('X-Drupal-Cache')) {
      if ($cached === 'HIT') {
        return FALSE;
      }
    }

I haven't thought too much on this - perhaps we can check if page cache is enabled here and return TRUE? We don't want to introduce more work for sites only using external cache.

@klidifia the change works - I have left a minor comment on the MR - please ping me if that suggested change looks good / works and I will retest.

This is indeed an issue for 2 scenarios.

1. Using formats with unrestricted HTML - this will allow all classes, so you can run into this issue when using the Full HTML format from the standard profile.
2. Using formats with restricted HTML and allowing <a class> for manually edited source tags (possible if you are not using the style plugin)

I have verified this issue on a clean install using both scenarios.

• Install Drupal with standard profile
• Add the anchor link button to the full html format
• Create new basic page node
• Change format to full html
• Insert some text
• Select the text and click the anchor button
• Save the node
• Inspect the source, the node is rendered with:

<p><a id="test" class="ck-anchor">This is my paragraph</a></p>

Expected result - it should be rendered as:

<p><a id="test" class="ck-anchor">This is my paragraph</a></p>

Producing the ghost anchor issue

• Edit the node created above
• Select the anchor and click the remove anchor button

The anchor is removed icon is visible in the WYSIWYG and editor can not remove it:

Using the ckeditor5_dev module I can see empty a tag remains in the view where the cursor is placed:

Viewing the model it looks like it persists with class attribute set

After saving the node the empty a tag remains in the rendered HTML.

<p>This is my p<a class="ck-anchor"></a>aragraph</a></p>

Editing the node again produces the same behaviour - there is a ghost anchor that can only be removed by manually editing the source.

Testing with the MR and it resolves the issue:

• the class is only applied to the editing down cast and the expected html when viewing the node does not have the class="ck-anchor" attribute.
• removing anchors after saving and editing works as expected - no ghost anchor remains

Looks good to me

Ah thanks @Greg - I just pushed with the suggested change - all good if we don't need it - thanks for the review!

Apologies for taking over a year to respond - I appreciate the review and the feedback.

Coming back to this after a year - the intent of setting the config to 1 is to maintain existing functionality. I believe its good practice to make sure newly introduced config includes an update to set the default value, otherwise we can make the schema nullable. Setting the value to 1 is not changing any existing behaviour.

I do find the UI confusing - and I think when you say "can check the box" when they install, that it looks like the other settings on the form are related / have control over this. That is my mistake for positioning the field next to that - I didn't really look close enough at the existing behaviour of that option first. That "Limit breadcrumb trail segments" is from #3174165: How to support limiting depth → and to be honest I'm still a bit confused by it - because I think the wording of the label and the functionality is a bit different 😅 But, I don't want to confuse this feature request with that functionality - I think it will be clearer if I make the UI clearer what this field does.

Perhaps it would be better to move this new field down into the Advanced settings - I prefer the UX of the form fields there that are enabled by a checkbox and then have the field value only shown when checked. I could refactor it so its like this:

I think that wording makes it a bit clearer too?

Found the core issue for this - not specific to this module 🐛 Entity displays need to depend on the module that provides base fields Needs work

Found an additional issue - when using a lot of templates the dialog appear to open at the end of the list, not the start as expected.

+++ b/js/ckeditor5_plugins/ckeditor_templates/sass/dialog.scss
@@ -0,0 +1,47 @@
+        display: none;

This is what is breaking the auto focusing on the dialog - when the dialog is opened it can't focus on the first radio button, so focusses on the checkbox creating a confusing scroll to the end of the list for the user

I think this is something that the module should be doing and the approach looks sensible to me - thank you.

I looks like an unintended change was introduced that broke the tests - I think we will also need more test coverage that the expiration is working.

Thanks for the review + feedback.

I am still really undecided on this on - at the moment it feels like this is not cloudflare modules problem to solve, and given it impacts trusted_reverse_proxy as well, I don't like the idea of both modules have to carefully keep an eye on core to see if priority is ever bumped.

At the same time, I proposed to fix it in redirect module 🐛 Broken redirect from RouteNormalizerRequestSubscriber when ajax_page_state present and $request->overrideGlobals() has been called Needs review - that too is not necessarily more stable as Symfony could change the behaviour at any point to make update other vars as part of overrideGlobals.

@rodrigoaguilera and @szy - we're both your scenarios using redirect module as well? I note there are other modules using server->get('QUERY_STRING') but interested if they could have any similar impact.

Overall - I'm glad the patch works but I'm still not convince what the permanent fix is.

ericgsmith → created an issue.

At some point clearing the plugin cache was removed from the save method on the form - not an issue for deployed config but it means when developing locally you need to clear the cache before seeing new templates, and if you delete templates you get a 500 error as it tries to load a plugin for an entity that no longer exists.

I have pushed a new branch to show the alternative - not wanting to high-jack the issue or current MR but I do want to show this as a possible alternative.

Needs additional work for an update hook to set registration_setting to visitors for sites currently using override_registration_settings - but leaving as needs review as there hasn't been any feedback on @realityloop's approach and I don't want to put any additional effort before getting feedback from maintainer that there is interest in this approach.

I was recently added as a maintainer and have committed and released the D10 changes.

If there are additional changes / features that you think are good for this module, please open follow up issues or join the contribution to existing ones. I think I will close this as outdated for now - please reopen if you are still interested and we can talk through some of the module issues.

Many thanks,
Eric

Test is failing as method only exists from 10.1 onwards

Added a test to show the error plus the a potential solution so setting to needs review although I note in the issue summary that there may be other ways to resolve this issue that I am keen to here ideas on.

ericgsmith → created an issue.

I also have a use case for extending this functionality that is a little different to what is described above.

We have a client where the default registration setting is Visitors, but administrator approval is required but wish to override it so that we do not register accounts through open id connect.

This is because we have very specific registration process that involves collecting additional information specific to the site, the site admin team then settings up user roles and invites or provisions the user in AD prior to approving the Drupal account, and then we rely on the Automatically connect existing users to sync the accounts. This works fine, however there is a edge case we want to prevent by overriding the registration settings with more control.

Users may already by have an account within the organisations AD through access to other systems - if they try login prior to registering a account is created and set as blocked - and there is not enough information for the registration to be processed.

I wondered if there would be appetite to specify the exact behaviour you want to override as - I believe that would cover the edge case above.

E.g. - introduce a new field in the settings "Who can register accounts through OpenID connect?" that is visible only when "Override registration settings" is selected. Allow options of Nobody, Visitors and Visitors, but administrator approval is required.

Thoughts?

That worked, thanks again.

Thanks @DamienMcKenna for the fix on the access!

Closing MR and hiding old patches so latest patch is clear.

Thanks @benjifisher for looking at this!

It is certainly an improvement that the formatter no longer displays the ignored "Thousand marker" option

I'm not sure I understand this - can you please clarify what the change is here you saw? It doesn't sound like something this patch changes but I may be missing something.

I have rebased the MR and applied the label change suggested from #44.

I have also created a draft change record https://www.drupal.org/node/3426241 → since this is a UI change for site builders.

Back to needs review.