TBH I never understood why Xss is not a service. Almost everything else in Drupal can at least be overridden/extended by a module. I find it a bit "un-Drupalish" that we say Xss::filterAdmin and not \Drupal::service("xss")->filterAdmin(...). This way, other modules like response_image could at least easily extend the list of allowed tags.
Probably too late to say this, but as much as I understand the reasoning in #50, I still think a solution like in #45 would still make sense *additionally*. IMHO there should be a chain: isSyncing() should internally rely on sth. like #45. Even in the issue summary it says "f.e.(!) when migrating". Now, the solution is ONLY about migrating. What if I just want to skip updating the changed date? I don't really know what setSyncing() might do now or in the future. I still think there's a valid reason to just want to skip changing the changed date. With the solution outlined here (AFAIU) I would have to pretend/claim that I'm syncing sth, when in fact I'm not.
Okay, I've looked into this further:
1) "Drupal\media_library\Plugin\views\field\MediaLibrarySelectForm" adds the "Insert selected" button at the bottom of the rendered view
2) Because of the "drupalAutoButtons" feature for modal dialogs which is enabled by default, any buttons which appear in the dialog's content area are moved out of there and into the actions bar at the bottom.
3) Views Infinite Scroll doesn't just add rows but adds a whole new rendered view page which includes another button. So after click "Load more" once, we have now: [Some rows] + [Button] + [More rows] + [Button]. The drupalAutoButtons features rips out all the buttons from the DOM and places them next to each other at the bottom.
4) And so on...
I found one solution (which is perhaps a bit better than just hiding the extra buttons with CSS) to insert the following line in js/infinite_scroll.js :
var $newRows = $newView.find(contentWrapperSelector).children();
var $newPager = $newView.find(pagerSelector);
$newRows.find('.button.media-library-select').remove(); // <== This is new
Of course, this is still not really the best solution because the extra buttons shouldn't be generated in the first place.
I guess we should wait for #3098132 to be resolved here because we actually want to only replicate the rows - not the whole view with HTML form tags etc. After this issue is resolved, I guess this issue here should get resolved automatically without any extra JS mumbo jumbo, because we won't replicate the insert button anymore but just the rows.
Another side-effect of this btw, is that the fade-out effect doesn't work for the media thumbnails loaded with "Load more". Usually if I can select only one media file (cardinality 1) and I subsequently select only one, all the other thumbnails will get a darken / fade-out effect. This also won't work because we add a whole new form so the items which need to be faded-out are now spread across multiple forms. This might also get resolved with #3098132 but perhaps extra work is needed. We'll see. Just wanted to note that this is sth. to keep track of.
Has anyone in the meantime found at least the reason for why this happens?
Ah, I found it: I needed to enable the Drupal core module "serialization". Weird, it's even listed in the info.yml as a dependency. I wonder how I was able to enable json_field without also enabling serialiation. Whatever, I'm closing this bug but perhaps it is helpful for others with the same problem.
rgpublic → created an issue.
Just for clarity's sake as I don't see this mentioned anywhere, but this happens, for example on CLI (e.g. Cronjobs etc) where $request->server->get('SERVER_SOFTWARE') obviously isn't available and returns "null".
Okay, I've filed:
https://www.drupal.org/project/drupal/issues/3418322 ✨ [upstream] [GHS] Allow CKEditor 5 support for containers: div, article, section, aside, etc. Postponed
I think it's better to not modify the focus of this bug too much. Feel free everyone to subscribe / continue discussion over there.
rgpublic → created an issue.
@hoporr: Yes, it won't work that way. CKE5 would need a dedicated plugin to define such a container DIV in the model and allow it to wrap other P tags. Unless such a plugin is written, you probably won't be able to make it work. Perhaps see also:
If I open the media library under Drupal 10.2.2 and then - still within the media library modal dialog - cause any AJAX request e.g. by changing the view exposed filters, all(!) page assets (like drupal.js etc) are loaded again causing many follow-up problems. This seems to happen because of:
if ($query->has('ajax_page_state')) {
$query->remove('ajax_page_state');
}
in MediaLibraryState.php. Will this problem also be solved by this bug and won't appear in Drupal 10.2.3 ?
@alexpott: +1 for 3.: I think it would make much more sense to offer the file extensions that are actually configured for the field. If I don't want allow PNG, these shouldn't show up in the file selector. So, IMHO, the default should be to set the accept attribute to
implode(" ",$extensions);
I'm unsure what we do about the mobile device capture though. Is it really necessary to have image/* to achieve that? What about HTML's "capture" attribute? Doesn't it trigger the "capturability" as well? Otherwise it wouldn't be possible to restrict the image file formats AND allow device capture at the same time, right? In any case, I think it should be a separate option then. Perhaps where you can even select the preferred capture attribute (front/back camera). If enabling this option then disables the ability to select allowed extensions, so be it.
Um, this commit changed jQuery.fn.selectAll to an arrow function. Due to this change, the selectAll function doesn't seem to work anymore it seems - causing JS errors. I guess the reason is that the semantics of "this" has been changed inadvertently. Using the arrow function, "this" is now pointing to a window and not to the JQuery object anymore.
If someone gets this error for a stand-alone CLI script. The quick solution (for me) was:
\Drupal::moduleHandler()->load('variationcache');
after $kernel->boot();
Hope this is helpful for some.
@Wim Leers: Well, to be fair though a similar approach is also already used for data-align, data-caption etc. Does it scale? Well, that's a very valid question you bring up. But you can see from these examples that it's not unheard of that simple isolated features bring in their own full-fledged text filter.
If we really want to make this more universal, then we'd probably have to create a general filter that turns all data-style-foo="bar" into style="foo:bar". We must make sure that there's a restricted list of "foo"'s this will work with of course - for security reasons. Other CKE plugins then might hook into this and just say hey, I've got a new attr=>style mapping here. Don't know if it isn't getting too complex though.
Unfortunately, "width: attr(data-width)" isn't yet allowed in CSS (https://developer.mozilla.org/en-US/docs/Web/CSS/attr). This would make it possible to solve it in CSS alone without any text-filter.
Well, AFAIU the "problematic" part tries to configure certain things for the existing media library image fields on install. I think this should just be omitted completely and users should instead by instructed to configure things themselves. I haven't even fully understood yet what exactly it is that would so sorely be missing if we just left that stuff out altogether. If we really want to keep this, then, I guess, we can't make it work with a simple static YML file. It's just not flexible enough. We'd have to run PHP code on install that looks for an image field etc. But, as I said, I don't think it's necessary. Just install the module without all that install-o-magic and let users configure their fields as they deem fit.
Seems like a duplicate of my issue #3390646. Hope one of those will be comitted soon.
Well, the main problem seems to be (IMHO) that way to many field templates (e.g. core/themes/stable9/templates/field/field.html.twig and many more) in Drupal contain a div tag when they should better use a span tag, I guess. A span tag would be more correct because a whole field, for example, is an inline element. There can be multiple fields in a row. As long as they are all using div tags, they can never appear inside a p tag :-(
Wasn't the complicated patch also due to the need to suppress the unwanted cart message? We now have this #3317738 so perhaps this could indeed be revisited?
@Wim-Leers: I understand & makes sense! Thank you anyway for considering & detailed explanation. My module might be a stop-gap solution for some people in the mean time until a full-fledged solution comes along.
@Wim-Leers:
Well, in CKE4 you could switch between P and H1.blue with just the paragraph style dropdown. Two clicks. You can't do that right now. Another difference is that say you have h1, h1.red and h1.blue. The new CKE5 style dropdown works more like a checkbox and not like a radio button. You could create h1.red.blue which is usually not what's desired.
The CKE5 style dropdown (IMHO, of course) still has a lot of very unpleasant usability flaws. Of course, they might be resolved in the future and it might very well be a sensible strategy to wait for these to be resolved - I don't want to argue against that. Nevertheless, I thought it would perhaps be a nice addition to allow to use the way more simple headline dropdown to be used for all those cases where you don't have a lot of different classes and styles and just want to have an experience similar to what we had previously in CKE4 - especially since the feature seems to be built-in already. It'd just need to be exposed in Drupal and would probably solve a lot of headaches for people trying to switch from CKE4.
@mrogers: That's right. This is not solved in 10.1. You can use my module (see above) to do just that.
@Anybody: Thank you for looking into this. I've just pushed a commit to the issue fork - hope it's correct. Still need to get used a bit to the new issue fork system. As you can see, I'd suggest removing the "once.remove('exposed-form', view.$exposed_form);" line altogether. Hope it doesn't cause any other problems. My line of reasoning was the same as detailed in #3133257: This module doesn't replace the exposed form. It only replaces the view's body. Therefore it shouldn't remove the once flag because it will subsequently cause a second/third/etc event handler to be reattached.
Um, why was this closed? My experience:
1) The problem mentioned in #3133257 was resolved as a duplicate of this one but is still valid
2) It happens not only with BEF but even with normal exposed filters
3) "Automatically load content" doesn't even have to be checked
To reproduce:
1) Create a view with exposed filter and the usual "Filter/Submit" button
2) Scroll down, click on "Load more". Wait for some additional view rows to be loaded
3) Open the Debug console's network manager, optionally filter just for XHR requests
4) Optionally change some filters and click again on "Filter/Submit". See how every request is now executed twice, because the event has been attached more than once.
NB: This bug unfortunately causes a dependency on entity browser even if media_directories_ui isn't even enabled. :-(
@marcoka: Hm, weird. I don't get this message but I don't think this is the cause of the problem because the message only says that sth. is deprecated - not unsupported. Sorry, I can't help you there :-( I've posted a screenshot on how it should look like. Are you using the newest Drupal version 10.x ? You're not using D9 right? I haven't tested this on D9. Perhaps others might be able to find out what's going wrong - I'm at a loss here.
There should be two values. One for word count and one for character count (see attached screenshot). Don't know why it isn't visible for @marcoka. It should work out-of-the-box. Perhaps change for any JS errors etc.? Sorry for dropping this here somewhat as-is. Unfortunately I cannot take care of this ATM as I'm inundated with other D10/CKE5 work. Like I mentioned before, if anyone wants to implemented limit/locking I'd suggest checking the example given here:
https://ckeditor.com/docs/ckeditor5/latest/features/word-count.html
@s_leu: Yes, it works now with the branch. Wonderful. Thank you so much! Would be great if you could release a new version. (I still think CKE5 should one day have a better way to define those dependencies so I'll live a comment there).
rgpublic → created an issue.
@s_leu: Yes, my site had the csp module installed and a Content Security Policy header. BUT: Usually if there's a violation against a CSP rule it should show up in the browser console. There was no violation. Just to be sure I uninstalled the module and checked that there's no CSP header now. As expected, this didn't help. The problem persists.
The error message "schema-cannot-extend-missing-item {"itemName":"drupalMedia"}" sounds as if it might be a loading order problem. Could it be this plugin here is loaded before the drupalMedia plugin? I found this here:
https://github.com/ckeditor/ckeditor5/issues/12199
There doesn't seem to be a real dependency mechanism in CKE5 between plugins. But the last comment suggest using afterInit() for such purposes. Perhaps you might want to try to move any schema extensions in the afterInit() method? Just a thought...
@amaisano: #122 is meant to be used in conjunction with paragraphs_asymmetric_translation_widgets. It has not functionality on its own.
Yes, I'm trying to use CKE5. I guess I setup everything correctly. See attached screenshots. Might be wrong though, of course.
rgpublic → created an issue.
For me it was sth. different still. I'm afraid it won't work reliably this way. Reason:
/admin/structure/media/manage/image/fields
Note that the fields here are freely configurable. Including their machine_name. This module should work (or even install) no matter what configuration you have IMHO.
Pretty sad. D9 EOL is approaching fast. There is a working patch. OneTrust just doesn't commit it. Yeah, there's cookiepro_plus, but it's way more complicated than necessary for most purposes. Considering that CookiePro is a commerical product this "underwhelming" level of support is a bit disappointing IMHO.
I just stumbled over this. It's a bit weird that the h3 tag and title seems to be part of the template "commerce-checkout-form--with-sidebar.html.twig". I guess this template should better care only about the layout. If the sidebar contains sth. different everything false apart. The headline should just be a part of the corresponding checkout pane IMHO.
Well, my very personal opinion is that it'd be the best to rely on features already in CKEditor5 instead of reinventing the wheel. My proof-of-concept module does that. I think relying on existing CKEditor5 features is also better performance-wise (in contrast to constant polling and analyzing the whole text). @calebtr is right though about the features we might lose. Although limit/locking could be implemented with CKEditor5's API quite easily and this is even documented in their docs:
https://ckeditor.com/docs/ckeditor5/latest/features/word-count.html (isLimitExceeded)
What's currently missing though is the different settings on how to calculate the exact count (with or w/o spaces and so on). One solution to implement this (if its decided that it's worthwhile to do so) and still rely on the original CKEditor5 API might me to subclass their plugin...
https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-wor...
...and modify the RegEx. Unfortunately it seems to be hidden away in a private property :-( Perhaps it might also be helpful to actually ask the CKEditor folks about whether they want to implement stuff upstream and/or at least make this property public. That might simplify things a lot.
We implemented this plugin properly - relying on the official CKE5 mechanism for counting words/characters. I'm donating the source code here. Feel free to copy all/everything as you see fit :-)
I wonder why the "Limit allowed HTML tags and correct faulty HTML" filter must be disabled and/or "Full HTML" allowed. Wouldn't it be sufficient if the "style" attribute was permitted for td/th ?
NB: The sandbox project does implement its own custom counting mechanism instead of relying on the existing CKE5 functionality which may or may not be what's desired. Just wanted to mention this.
Aah, I finally found it. Sorry for the spam, but perhaps this is helpful to others. I didn't notice it in the patch but the most important change (after cleaning up the schema yml) is the *change of the filename(!)*. The filename must be "[modulename].schema.yml" not "ckeditor5.schema.yml". Otherwise your schema will *override* the schema with the same name in core (core/modules/ckeditor5/config/schema/ckeditor5.schema.yml). All the problems arise from this single issue. So make sure after applying the patch, that the original "ckeditor5.schema.yml" really isn't there anymore.
PS: It seems I could make it work after adding this to config/schema/ckeditor5.schema.yml. I suspect this section didn't exist in D9's core/modules/ckeditor5/config/schema/ckeditor5.schema.yml and exists now in D10. That's why it's missing. What I still don't understand is why the configuration about all the other plugins has been included in this plugin (a mistake?) and why I now doesn't work when I'm removing it. It should be still available core/modules/ckeditor5/config/schema/ckeditor5.schema.yml. Weird.
# Plugin \Drupal\ckeditor5\Plugin\CKEditor5Plugin\CodeBlock
ckeditor5.plugin.ckeditor5_codeBlock:
type: mapping
label: Code Block
mapping:
languages:
type: sequence
orderby: ~
label: 'Languages'
constraints:
NotBlank:
message: "Enable at least one language, otherwise disable the Code Block plugin."
UniqueLabelInList:
labelKey: label
sequence:
type: mapping
label: 'Language'
mapping:
label:
type: label
label: 'Language label'
language:
type: string
label: 'Language key'
This bug is unfortunately catastrophic for me. After uninstall, I still get this error. What I don't understand is... the config schema is in this file:
core/modules/ckeditor5/config/schema/ckeditor5.schema.yml
Why isn't Drupal picking it up? How can I repair / reimport the schema? It seems the whole trouble began because there was a lot of unneccessary schema included in the config file of this module, right? That's why the patch has so many removed lines...
Sorry to ask but is this module now D10 compatible with these changes? Could a new version perhaps be released?
@Tichris59: But is this a new problem due to the patch? Or is this a general problem/bug with vppr. In the latter case, this should perhaps be files as a new bug, I guess...?
I hope a maintainer can finally review #122. It should not have any negative effect. I'm not just claiming this. Proof:
if ($host->isTranslatable()) {...
Usually, the host paragraph should be set to untranslatable. Only with async paragraphs, the host is set to translatable. That means, with normal synced paragraphs, this won't run any additional code.
Next:
$this->isTranslating
is replaced with
!$this->allowReferenceChanges()
however allowReferenceChanges is modified by the patch to:
!$this->isTranslating || $this->supportsAsymmetricTranslations();
and supportsAsymmetricTranslations always returns "false".
Thus, isTranslating is effectively changed to:
!(!$this->isTranslating || false)
This can be simplified to:
!(!$this->isTranslating)
This can be further simplified to:
$this->isTranslating
Which is what the original code was.
Conclusion: Patch #122 literally does not change *ANYTHING*. It is *mathematically* equivalent to what is there before. The risk of including this should be zero.
rgpublic → created an issue.
I think the dependency should be removed nevertheless. It makes it very problematic to upgrade to CKE5 on D9.
Thanks for the pointer. I will certainly check out what comes eventually out of this and if it is useful.
For the time being I've created a simple module which allows to customize the headings exactly like it was possible in CKE4. Perhaps it is also useful for other users as a bridge technology:
rgpublic → created an issue.
I fixed this by adding this to the admin stylesheet:
.ck.ck-toolbar.ck-toolbar_grouping > .ck-toolbar__items {
flex-wrap: wrap;
}
Perhaps this is useful for anyone.
What does "I think it is not backwards-compatible to the older versions of the shy-module" mean? After all, in the HTML code that's finally stored in the field it should be just a ­ entity, right? So if I open such a field with the new CKEditor5 then I can see/edit the shy's with the new module, right?
BTW: Thanks from me as well for the module. +10 :-)
It'd be even better IMHO if svgo was supported:
@Wim Leers. Very good question. I asked myself the same thing. The answer is: The Drupal Media Library plugin has no edit feature. If you click on the "Insert media" button a dialog is opened and you can select a media file. The media file is then inserted at the cursor position. However, there is no real way to "edit" the media file. You cannot double-click and if you highlight the media item and click on the button again, just normal dialog is opened again but the current file is not preselected. It's okay-ish I guess for this use-case. It would be nicer though, if the library would preselect the file. If you use sth. like media_directories this gets worse because you have to select the same folder again. Long story short, there currently isn't any information flow from CKEditor to the dialog - only from the dialog back to CKEditor. That's why this feature isn't needed.
The only argument against it would perhaps be the symmetry to the CKE4 version. But I don't really oppose the idea. Just a thought.
I sincerely hope, "No Drupal dialogs in CKEditor anymore" doesn't mean support for it will be removed completely...? After all Drupal.ckeditor5.openDialog() in core/modules/ckeditor5/js/ckeditor5.js exists and for some use-cases it's in fact very nice to have this option still available. Not everything can be crammed into a tiny bubble and sometimes you need really need Drupal's backend features, form validation, form fields etc. I have nothing against the link feature in a bubble, though. Just wanted to argue for keeping the dialog feature alive.
But also, I'm just unsure this is a good idea to just expose settings for everything... Disabling this programatically isn't really complicated but I know writing code isn't an option for everyone.
Well, IMHO, Drupal from A to Z is only about making everything configurable. If you look at Views, many configuration pages in Commerce etc. etc. It's a flood of checkboxes, dropdowns etc. The problem I guess is that Drupal is missing sth. viable between writing a whole new module in PHP and just setting obscure checkboxes. Sth. like "about:config" in Firefox browser, for example. But, for the time being, I still think it would be nice if this message could easily be changed, translated etc. It would perhaps be sufficient if there was a Twig template suggestion to override this message. This would also make it possible to make the message dynamic (perhaps depending on the item just added etc.). And if this template is empty (to be defined what that means - perhaps a test with trim() equals empty string) the message is suppressed altogether. If there are individual template suggestions per order type, this issue here could be solved elegantly without any new UI but also without the need for writing modules. Just an idea.
PS: It would already help if the credential fields were optional and could be left empty.
+1. Coming from the commerce_braintree module and was pretty disappointed that this doesn't work out-of-the-box here. Als makes it unnecessary difficult to automatically switch between development and live systems.
@aiphes: Exactly. You can open and read that file if you don't believe me. You will find that contains only things that are needed for *development*. It will significantly slow down your live website. It disables caches, enables debugging headers and so on. It will also leave your site vulnerable to some attacks. You *can* run with this enabled. But it's not recommended.
Um, sorry to interrupt the party, but I'd like to note that usually the patch is not needed for live websites. Make sure the development mode is not active (i.e. the line "include $app_root . '/' . $site_path . '/settings.local.php';" is commented/not enabled in sites/default/settings.php). Development mode should not be enabled for live websites! If you run into this problem with development mode enabled, make sure "http.response.debug_cacheability_headers" is set to false in sites/development.services.yml. Make sure you clear the cache after any changes in these files. These things should usually avoid this error - no need to patch anything.
@emclaughlin: No, I also don't have it, but I guess it probably won't help you anyway. The error message is almost universal and always happens when the Plugin is not found for some reason or another. Did you read my previous comments? They might be helpful for you. I also struggled a lot with this. I'd recommend checking the MODULE_NAME.ckeditor5.yml file and make sure you are compiling (yarn build) and deploying the .js file correctly.
I wonder if it wouldn't be nice to have a "shortcut" so instead of this:
data' => [
'type' => 'blob',
'mysql_type' => 'json',
'pgsql_type' => 'jsonb',
]
just this would be possible:
data' => [
'type' => 'json',
]
Yes, that's right. Although I wonder how many contribs actually use this. TBH, I only discovered this method due to this bug. I couldn't find any documentation on this. Neither any info on the related Drupal.ckeditor.openDialog.
Especially due to the missing existingValues param this method (in contrast to the version for CK4) is almost useless, I guess. I resorted to a direct Drupal.ajax() call for now.
PS: Finding now #2810723 I wonder... Do we really need an additional option for this? If #2810723 ever gets implemented (switching off the cart completely per order-item), I'd assume the corresponding message disappears as well without any further configuration...?
@AndyD328: Got you. I've subscribed and commented over there. Thanks for the pointer.
Coming from https://www.drupal.org/project/commerce_cart_redirection/issues/3218501 ✨ Suppress the " added to your cart" message on redirect? Closed: won't fix I have 2 questions:
* Is it really needed to have an option to set this per order-type? What is the use-case if I may ask? I have nothing against it, though. Just wondering...
* Could anyone of the core maintainers comment if this is feasible? Otherwise we'd have to find an out-of-core solution. I definitely think solving this in core makes more sense. At least as long as it's as difficult to switch the message off as it currently is.
BTW: Should we perhaps set this on "Needs review" to move this forward?
I still think this is badly needed. It's not really a great solution to just hide the message via CSS as this will suppress ANY (probably important) error message. There's not an easy other way either to get rid of this message for template/theme authors. And I don't see any likelihood to have this solved in Commerce Core in the forseeable future.
I've attached a patch who tries to solve this a little less aggressively than what was originally propose in the issue description. I tried to use best practices to make this as little intrusive as possible. I'm using a service decorator and I'm only decorating the problematic function that really generates the unwanted message in the first place. If in the future the Drupal Commerce team includes any other important functionality in the Event Subscriber that won't get thrown out.
In addition, I've made this behavior completely configurable, of course. What are y'all thinking about this? Good idea? Safe to go in?
Ah. I see, I'm wrong. Page Cache seems to use $expire in core/modules/page_cache/src/StackMiddleware/PageCache.php, storeResponse. But I also found that only -1 ends up in the cache_page table. Weird.
I found debugging stuff like this extremely hard. I had some success going to "lib/Drupal/Core/Cache/CacheableMetadata.php" and e.g. doing a backtrace in merge() when an unwanted max-age:0 is about to get merged in.
Besides that, I still think this issue is very valid. This module currently only fixes the fact that a default Drupal installation doesnt properly bubble the Max-Age to the Cache-Control HTTP header. It doesnt do anything about the fact that the page cache doesnt support time-based expiration *at all*. Of course, I could write another module for this. But I think it might make sense to integrate it here, so we don't get to a point where we require a whole library of cache fixing modules to get a decent working caching mechanism in Drupal...
@tacituseu: But you forget that this might be very helpful for people who currently cannot upgrade and need this.
Ah, thanks @tacituseu. See it now. Did a mistake while adapting the patch to 8.2. It seems to work now. I've attached it in case anyone needs this.
Hm. Don't know if I'm mistaken but patch #97 seems to add the checkbox to every field - no matter whether it actually consists of multiple values (cardinality!=1) or not.