Code-wise this LGTM, still there might be an impact on performance, as each view is now executed twice
Yes, this is indeed a problem, but I am unsure how to fix it. As @starlight-sparkle in #3 correctly says, that this is probably the only way to check whether the output is really empty or not.
I thought about caching, but I don't think that is a good option in this context...
grevil → changed the visibility of the branch 3453071-dynamic-caption-better-custom-tokens to hidden.
IMO, this issue shouldn't be about improving the token compatibility, but rather be a feature request to add compatibility to use the referenced entity fields as a caption source.
Ok adjusted everything accordingly! For some reason the KernelTests time out. I had a similiar issue in another core issue. Should be hopefully fixed with the next rebase.
Added a few comments. Also, the issue from #37 ✨ Stripe Express Checkout Element Integration Active is back.
Probably because the VAT is once again part of the line items, even when it is set as included (see #38).
There are also still multiple threads open on the MR
Found two more issues. The settings still needs to be tested manually.
Static patch attached.
Ok, I finalized the styling and fixed an inconsistency with the action buttons being nested unnecessarily.
- All the buttons now have the "button--small" class when the field cardinality is 1. Otherwise, we stick to the original styling of the "remove" button which is provided by core for cardinality != 1.
- All the buttons now reside inside a container, which prevents the buttons being all over the place.
- "add another item" and "open entity browser" are still under each other. I don't really see a problem with that and to have the "open entity browser" above the table isn't easily done.
We are not generating the iframes in this module. This module simply provides content to render inside iframes by third party sites.
I created information noticing the user about the usage of "sandbox" in the README file.
Whoops didn't see this. We got tests for this now.
Duplicate of 📌 Add example and Test for allowedReferrers Active .
I just auto-generated a first approach using AI. I haven't checked the code yet. If we need this feature soon-ish, we can expand on this.
But for now, the allowed referrer should be enough.
Fixed! Also adjusted the required parameters validation, as it wasn't correct.
Thanks again @kksandr very informative fixes as well!
Tests are green! Merging.
Very nice! LGTM!
Let's wait for the tests to pass. People had problems in the past with newly injected dependencies on already existing services. We never had a problem with that. Simply clearing all caches resolved the issue, although we introduced running "DrupalKernel::rebuildContainer" inside an update hook once, which fixed the issue for people having these problems. I am thinking whether we should introduce such an update hook, but I personally find it very unnecessary.
RTBC.
Regarding the many tests we have, this fix is fairly "save". I'll merge it.
The current approach should be the option we have to fix this issue.
It will have problems with terms like " test ", "test." or "test," but I don't think it is even possible to save drupal entities having a label with a trailing whitespace? And using "." or "," inside the label is also super uncommon and would have let to a problem before.
Nice! So let's add the 4-options select option for "Partial word match":
Only match whole words (default)
Match partially at end of words
Match partially at beginning of words
Match partially anywhere
I don't think, that is related to this issue, but we can create a follow-up issue for that in the future.
@kksandr one last thing, we can remove "getTexformatsUsingMentions", but there is also a test covering the method in "CkeditorMentionsEventsTest" could you add a replacement covering "isFormatMentionable"?
Only if it is done quickly.
Changes are looking good! Thank you! :)
Commented a few more things, otherwise this issue looks good to go afterward!
Thank you, @ressa!
We also have an OpenCollective now, if you want to tip us a coffee! 😉☕(https://opencollective.com/drowl).
Wouldn't that be a great little feature and quite useful?
Indeed! Sounds interesting!
Totally makes sense! Thanks for the fix!
I added a test to reproduce this issue. Here is the test output without the changes by @prudloff in place:
There was 1 failure:
1) Drupal\Tests\glossify\Unit\GlossifyBaseTest::testParseTooltipMatch with data set "set32" ('<div class="glossify-exclude"...ified.', [stdClass Object (...)], true, true, 'tooltips', false, '', '', '<div class="glossify-exclude"...ified.')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'...><a>RT</a></div> Here is <span title="Lorem Ipsum is simply dummy text of the printing and typesetting industry. Lorem Ipsum has been the industry's standard dummy text ever since the 1500s, when an unknown printer took a galley of type and scrambled it to make a type specimen book. It has survived not only five centuries, but also the leap into electronic typesetting, remaining essentially unchanged. It was popularized in the 1960s with the release of Letraset sheets containing Lorem Ipsum passages, and more recently with desktop publishing software like Aldus PageMaker including versions of Lorem Ipsum.">RT</span> that should be glossified.'
+'...><a>RT</a></div> Here is RT that should be glossified.'
/var/www/html/web/modules/custom/glossify/tests/src/Unit/GlossifyBaseTest.php:35
The patch is incorrect. Could you please provide proper steps to reproduce? $ignore_tags shouldn't be undefined. It is one of the parseTooltipMatch parameters and is properly passed through $this->settings['x_ignore_tags'] to each submodule respectively with a proper default value defined in the class doctrine annotations.
LGTM! Thanks, @ressa!
We were in need of such a feature for a few customers, so I took the time to finish this. It works similar to the approach by @joao.ramos.costa, I appreciate the work and urge to give him credit, once this is reviewed and merged.
You basically have to create a Text (plain, long) field called "field_glossify_synonyms" on the bundle you wish to enable synonyms on. Then when you create a bundle, for example "Technology" you can define "Tech" and "IT" as their synonyms in the "field_glossify_synonyms" text field.
Now when you type either Technology, Tech or IT, all of these terms will be glossified and link to the main node (if link is enabled).
We can improve on this in the feature through adding support for "Synonyms" in the future. I haven't used the module yet, and I am unsure whether it would add much overhead.
Ok, once the new plugin system is merged in 📌 Allow to filter nodes by bundle Active , this can be reviewed and merged as well.
POSTPONED on 📌 Allow to filter nodes by bundle Active .
Great stuff! Exactly what I had in mind! Also didn't know about "_editor_get_formatted_text_fields". Good stuff!
Although, I am not a big fan of the in memory caching for "mentionableFormats". It is a good idea to cache the formats, to have less performance overhead, but I don't like using a class variable for caching. I'd rather use the Drupal caching system instead!
Otherwise, LGTM!
Note to myself, I already implemented this feature in https://git.drupalcode.org/issue/ckeditor_mentions-3544907/-/tree/941420.... I just have to use the plugin instead of hardcoding a url query parameter.
Actually, I would really like to merge this feature as is. What do you think about this @dinazaur?
This change would theoretically break compatibility with any text fields that do not extend "TextItemBase". But all core text fields and any other important contrib text fields I know, are extending TextItemBase anyway.
Remove !$user->user_picture->isEmpty(), since we already have exactly same check !empty($user->user_picture->entity)
Agreed. The "image" field type uses "EntityReferenceItem::isEmpty()" which is defined as follows:
public function isEmpty() {
// Avoid loading the entity by first checking the 'target_id'.
if ($this->target_id !== NULL) {
return FALSE;
}
if ($this->entity && $this->entity instanceof EntityInterface) {
return FALSE;
}
return TRUE;
}
So it would also return false when the target_id is not null, skipping the entity completely. Might even make sense to create a core issue for this? Unsure.
Thank you @askibinski!
Agreed, I'll start here once 📌 Allow to filter nodes by bundle Active is merged.
I messaged him on Slack.
MR conflicted with current dev. I rebased it and rebuild the js. We can merge it now.
Ok, I see, that I can not persuade you into fixing 🐛 Memory size get exhausted if we try to use this module in system which has 10,000+ users Active here. Therefore, I agree this being a minor improvement.
I don't like the idea of using query parameters. We need to leverage plugin system instead
Adjusted accordingly, please review!
Hey @dinazaur, filtering by bundle and especially the provided entity query improvements are quite important as this module is unusable on large scale sites. I agree with you, that this should normally be a part of 🐛 Memory size get exhausted if we try to use this module in system which has 10,000+ users Active but I didn't want to handle the rebase errors.
Setting back to "Needs work" as the tests fail, sorry didn't see that.
You are welcome!
All done now. Please review!
POSTPONED on 📌 Allow to filter nodes by bundle Active .
The query limit fixes issue 🐛 Memory size get exhausted if we try to use this module in system which has 10,000+ users Active is fixed here, so I don't have to bother with rebasing errors.
Yea setting a hard limit here doesn't make much sense. Especially when we already got a "dropdownLimit" setting. Since this module uses AJAX to get the entities and the entity query does the filtering, we can simply use the "dropdownLimit" setting for the limiting.
I implemented this inside 📌 Allow to filter nodes by bundle Active , because that issue has a bunch of other code changes and I don't want to bother with rebase errors.
Done, please review!
Should this issue be closed, then?
I mean there are still a few phpcs fixes to fix:
FILE: /var/www/html/web/modules/contrib/ckeditor5_mentions/src/Element/MentionsIntegration.php
----------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------------------------------------------
61 | WARNING | Editor::loadMultiple calls should be avoided in classes, use dependency injection instead
----------------------------------------------------------------------------------------------------------
FILE: /var/www/html/web/modules/contrib/ckeditor5_mentions/src/Utility/HtmlHelper.php
-------------------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
-------------------------------------------------------------------------------------
80 | ERROR | String concat is not required here; use a single string instead
97 | ERROR | String concat is not required here; use a single string instead
98 | ERROR | String concat is not required here; use a single string instead
-------------------------------------------------------------------------------------
But yea, I guess it can be closed again.
Thanks @avpaderno!
LGTM!
Did you simply run "npm run dev" or what are the changes here exactly?
Definitely makes sense! I wonder if we could specify this even more through checking whether the field widget actually uses CKEditor.
This way, we avoid the Text fields that don't even use CKEditor. If this introduces too much overhead, we can always use the current approach as it improves the current state a LOT.
Done!
Please review.
Created a follow-up issue to remove the master branch and set 2.0.x as the default branch here: 🐛 Remove master branch and make 2.0.x the default branch Active .
Setting back to "Needs Review" for the small EntityTypeManager fix.
Ok nevermind, I just realized, that the changes here were merged into both master and 2.0.x, and they were fixed by the maintainer in 2.0.x but not in "master".
Since "master" is the default branch I got quite confused.
I guess it still makes sense to merge https://git.drupalcode.org/project/ckeditor5_mentions/-/merge_requests/4, but it is not that important anymore.
Yea all good, I'll provide fixes in https://git.drupalcode.org/issue/ckeditor5_mentions-3353151/-/tree/33531...
50% of the changes here make no sense and completely blow up the module.
This issue introduced a services.yml entry, targeting a non-existent class:
ckeditor5_mentions.editor_manager:
class: Drupal\ckeditor5_mentions\EditorManager
arguments:
- ['@entity.manager']
(Not to mention, that there is no '@entity.manager' service)
NOTE, that this does not happen on a vanilla Drupal installation. For me, this error happens on a side with an enormous amount of other modules installed. Eventually, this module doesn't even cause this behavior. I will provide further feedback to this.
Both modules do not support specific node(entity) restriction
Not 100% sure what you mean with this, but this module supports filtering by an entity type by bundle. E.g. "only mention nodes of type article"
Changes LGTM!
Please review new MR with the regression fix.