Great job. Thanks for your work on this!
Thank you @dcimorra for the patch for Drupal 10.
I see that the MR is for the 7.x-1.x branch. Can you please start a new branch that is compatible with D10+ and move the MR there so your commit doesn't make changes to the 7.x branch.
I also noticed that the README was removed. Would you be willing to add one, and include a note that helps people understand that the core D10/11 lets you flush individual styles, but this module gives users a Flush All feature.
My opinion is that we should set the weight to 1 by default, not just for new module installations but also for existing sites.
Part of the goal with this issue is to fix the problem where the honeypot field is being ignored by semi-intelligent bots. The most likely culprit is that a weight of 100 typically means it is rendered after the submit button(s), and so bots ignore it. If we leave the default weight at 100, this module continues to be useless against semi-intelligent bots.
I imagine many site admins don't read through every changelog for every module update, and so they will miss a message that says: "New feature: you can set the weight of the field which is currently 100. It's recommended to set this to a weight of 1." So unless they make the change (either via manual config, or updating the variable in deploy scripts), the module will continue to be useless against semi-intelligent bots.
Moving back to 2.2.x
Per @Input's comment in #34, attached is a patch which makes weight configurable.
This patch is for 7.x.
hargobind → changed the visibility of the branch 7.x-2.x to hidden.
hargobind → created an issue.
Maybe this can help? In particular, check the comments under the accepted solution.
https://stackoverflow.com/questions/8044583/how-can-i-move-a-tag-on-a-gi...
The only thing is, I don't know if there are any restrictions on Drupal's implementation of GitLab that allows force pushing a change (e.g. git push -f origin <tagname>
).
hargobind → created an issue.
@TR could you look at fixing the tag for 7.11.
It appears to be based on a feature branch commit (https://git.drupalcode.org/project/entity/-/commits/24c6c4f1ce838f4439b5...), rather than the 7.x-1.x branch (https://git.drupalcode.org/project/entity/-/commits/7.x-1.x).
Patch to address this error.
Question: on the MR, there is an option "Delete source branch when merge request is accepted."
I want to make sure I'm correct in understanding that source branch = MR/feature branch, and destination branch = the 7.x-2.x branch... right?
Am I correct that checking it means that once the MR is merged, the feature branch I created for this MR will be deleted?
@TR: There you go.
Thanks to pipeline tests, I found a PHP 8.2 bug in the new code. All tests are passing now.
This was my first time using gitlab's MR. (I didn't realize the community switched away from the patch-based approach in the last few years.) Let me know if I missed anything.
hargobind → changed the visibility of the branch 3460676-php-8.0-deprecate to hidden.
The error above applies to the 7.x-1.x branch.
hargobind → created an issue.
This is a duplicate of
#3118289: Passing the $glue and $pieces parameters in reverse order to implode has been deprecated since PHP 7.4 →
While the error might be different, the patch is essentially the same.
Similar to #35, I experienced the same error when running on PHP 8.x.
Attached is an updated patch to fix this.
Also resolves the errors on my sites.
@rszrama: Any sense of why these values are NULL to begin with?
The "data" column on these tables is NULL for most rows on each of those tables on my sites. Not sure what the expected behavior is in the code, but that's what I'm seeing in the database.
Confirming that #2 fixes the issue.
The issue was reproducible for me during site cache clear.
#16 works for my sites as well.
Since this issue priority is Major, marking RTBC unless @poker10 or other maintainers have something to say about it.
The commit in #16 can go in now, and then someone can work on the backport for jShrink at a later date if it's still important.
Same error in a couple other files:
lib/Container/MultiWildcardData.php
lib/Container/MultiWildcardDataIterator.php
lib/PluginSystem/PluginMethodIterator.php
Attached a patch for these additional files.
I just ran into this as well.
With no image file, a PHP error was being thrown during the AJAX request for
https://www.example.com/scald/library_dnd
Warning: array_flip(): Can only flip string and integer values, entry skipped in array_flip() (line 175 of /includes/entity.inc).
Or, if you have the Entity cache module enabled, this error will appear instead:
Warning: array_flip(): Can only flip string and integer values, entry skipped in array_flip() (line 84 of /sites/all/modules/entitycache/includes/entitycache.entitycachecontrollerhelper.inc).
And another error in the scald_image provider module:
Warning: Attempt to read property "uri" on bool in scald_image_scald_fetch() (line 205 of /sites/all/modules/scald/modules/providers/scald_image/scald_image.module).
Those errors were causing the Scald sidebar DND panel to not load on my site.
Requiring the scald_thumbnail
field seems like the recommended approach in my tests.
Fix for accidental overwriting of string in last patch.
Attached patch uses the proposed resolution.
hargobind → created an issue.
Ohh right, of course ;-)
Thanks for fixing this.
Now that this has been committed, I'm marking ✨ Support CKEditor 4.18 Closed: duplicate as a duplicate.
Before we consider this closed, one concern was brought up in that issue in #5.
The 4.18 release of ckeditor has dropped the wsc spellchecker plugin.
This means that the wysiwyg will fail to load on any profile which has the Spellchecker turned on.
An admin just needs to update their wysiwyg profiles but should there be an update hook to turn that off automatically?
I didn't have the spellchecker installed, so I didn't run into the same issue. Can someone here try to replicate this and confirm whether it is/isn't an issue?
Now that ✨ Update to CKEditor 4.21.0 Fixed has been committed and released, I'm marking this issue as a duplicate.
I've been running a few sites with patch #2 in production for a long time and never ran into #6.
I added @davidrobinson_pw's comments from #5 to the issue where the fix was committed. Please continue the discussion there if the upgrade with spellchecker is still a problem.
Patch #2 works great and applies against 7.x-1.x.
Considering that thousands of images could be uploaded using this module, I consider it really important to have a sense of file organization so that the public files directory doesn't get overloaded, and this patch fixes that concern.
Patch #2 solves the issue where an uploaded image isn't correctly inserted even though my CKEditor body field shows a visual representation that it is.
Patch applies cleanly to 7.x-1.x.
The patch in #2 works perfectly (even 8 years later).
The patch still applies cleanly to the 7.x-1.x branch as-is.
Get ready for a novel...
1. Other important flags
To further @anthonyroundtree's idea of allowing users to pick those attributes:
What if the URL being embedded isn't a video? We need to consider that users might be embedding a form (allow-forms
), or a webpage that would load a popup (allow-popups
), or want modals (allow-modals
), etc.
Here's a web.dev article that explains some of the flags in layman terms.
By default, we should really be enabling most of the flags mentioned in the MDN article because we can't assume how a site editor will be using their content.
If someone here were to code a picker interface, I'd like to see a picker that shows all flags, including some "default/recommended options", and "lesser-used options" or options that require some level of caution. Somewhere in there it should link to one or both of those articles above for admins who want to educate themselves.
2. Alternate solution: disable sandboxing
We can just turn off sandboxing entirely for whitelisted domains.
I understand this may be an unpopular opinion, but here are my reasons:
- Until a CVE is announced for CKEditor 4 stating that all versions prior to the sandboxing behavior (released in v4.21.0) are insecure, we should treat the sandboxing as a "feature", and we should do our best to be as backwards-compatible as possible, which means either disabling the sandboxing feature, or enabling all (or nearly all) flags.
- For the CKEditor team (thank you for your service) to add the sandboxing feature enabled by default, especially as a minor version, it feels like it's forcing our hand and causing a downstream regression/bug, and it's making us choose an acceptable security policy that we (the people in this issue and maintainers of this module) deem appropriately secure enough, but also wide enough not to break the content behaviors of an unknown number of websites out there.
- We can't predict how a site admin wants to restrict the content, or that they even understand the security implications, or the bugs that can be introduced by too-restrictive policies
- Making a change to sandboxing here will only affect content that is edited going forward. (See my discussion below: 4. Beware of mass-updates to content)
- It has already been mentioned that this module is unsupported. So adding new complexity will make it harder to get a patch in.
- Unless we chose a very permissive set of flags (i.e. nearly all flags) we should expect someone here to put even more energy into coding a picker-interface for all sandbox flags, which would make it harder to get a patch in.
Disabling of sandboxing can be accomplished in a patch.
Instead of:
return {sandbox: "allow-scripts allow-same-origin"};
Do this:
return { };
However, there's a minor bug with that, which is that if you first embed an iframe with the URL "bad-example.domain/watch/123", then edit the source code and switch the URL to "good-example.domain...", the "sandbox" attribute stays there and doesn't get removed. I don't think this is a bug in the patch, but rather in how CKEditor handles the cleanup of iframe attributes. The workaround is to remove the iframe and re-embed it with the correct URL.
3. Case-sensitive problem in the patch
The line that checks the src
attribute:
if (iframe.attributes.src.indexOf(Drupal.settings.ckeditor.allowed_domains_in_iframe[i]) !== -1) {
That will fail if there is a case-mismatch between the source and whitelisted values.
Both "src" and the whitelist value need to be compared using the same case.
4. Error with YouTube videos without the "presentation" flag
As far as just videos, in addition to the two attribute flags "allow-scripts" and "allow-same-origin", we should consider adding a third:
allow-presentation: Allows embedders to have control over whether an iframe can start a presentation session.
I'm on Chrome 116.0.5845.111 on Windows 10, and loading a page with a YouTube video without the presentation flag throws this error:
cast_sender.js:84 Uncaught DOMException: Failed to construct 'PresentationRequest': The document is sandboxed and lacks the 'allow-presentation' flag.
at V.initialize (https://www.gstatic.com/eureka/clank/116/cast_sender.js:84:87)
at chrome.cast.initialize (https://www.gstatic.com/eureka/clank/116/cast_sender.js:98:162)
at w9.init (https://www.youtube.com/s/player/16f9263d/player_ias.vflset/en_US/remote...)
at Owb (https://www.youtube.com/s/player/16f9263d/player_ias.vflset/en_US/remote...)
at Ewb (https://www.youtube.com/s/player/16f9263d/player_ias.vflset/en_US/remote...)
at https://www.youtube.com/s/player/16f9263d/player_ias.vflset/en_US/remote...
at chrome.cast.ea (https://www.gstatic.com/eureka/clank/116/cast_sender.js:100:385)
4. Beware of mass-updates to content
The behavior introduced by the new version of CKEditor only affects sites that have been upgraded to CKE 4.21.0, and then only for pages with an IFrame that have been edited since that upgrade. To that end, a hook_update_N()
has been proposed in the patch.
I get the intention behind making all content on the website secure, but is it really this module's job to alter potentially thousands (or millions) of pieces of content? The update could potentially take a really, really long time for so many pieces of content.
What if I have node types containing a body field with a certain input format that is set to not display a wysiwyg editor? Those nodes will be erroneously altered by a mass-update.
Also, what if a big site like YouTube or Twitter/X implements a new flag that results in console errors on all your old content? Then the flags that we changed during the update will need to be changed again and the update rerun.
If a person wants to make all past content on their site secure, there are other modules that can do a find/replace within the content (such as
Search and Replace Scanner →
). Or better yet, don't even touch the database content, and instead use a Text Filter module to parse all IFrames and output specific sandboxed attributes (or no attributes) depending on the source URL/whitelisting. (I recommend building a Text Filter using code like in @Spokje's hook_update_N()
that iterates over DOM elements using DOMDocument()
. I already have code that does something like this if anyone wants it).
Wonderful. Thank you all!
Due to a code change in 🐛 Convert settings from serialize to json_encode Fixed , this patch needs an update to account for the change in content_access.install.
I also took this opportunity to clean up the new schema table definition and move it into content_access_schema()
.
The two differences between this patch and #5 are:
1. Increase the hook_update_N()
version number to 7104.
2. Move the the new schema code into content_access_schema()
.
hargobind → created an issue.
Attaching a patch which makes these changes.
Note that since title has its own column in the {mailchimp_signup} table, a DB update is required.
hargobind → created an issue.
Ultimately that decision rests with the module maintainers. But this seems like a sensible change to me.
It would maintain a bit of feature parity as far as how data is encoded for ACL and content_access.
It's also safer from a security standpoint since serialize()/unserialize() is prone to remote code execution vulnerabilities.
@laryn would you like to contribute your patch or do you want me to?
Accidentally included the ['user_list'] in the previous patch.
Attaching a patch with this one-line change.
hargobind → created an issue.
@danyg this is a great patch!
The logic in this patch is sound, and it works like a charm on my sites.
I have made a handful of modifications to make the wording clearer and fix code formatting.
There is one concern here which is that the original change made in
🐛
"content_access_author" grant does not react to role changes
Fixed
that was released in 7.x-1.2 has been public for a few months, and some site admins may be depending on the notification message in that release to indicate that permissions need to be rebuilt. Since the change in this issue removes that message, I think it's better that content_access_silent_rebuild
defaults to TRUE. And since the changes here only update certain nodes, enabling this by default shouldn't cause performance problems. This change has been included in my patch.
hargobind → created an issue.
Attaching a patch.
hargobind → created an issue.
Fix to #84 to rename the new options function to xmlsitemap_get_changefreq_form_options()
.
This is a duplicate of 🐛 db condition and join causes fatal errors in PHP 8 Fixed , but attribution should be given for the work done.
For now, I am providing a patch which seems to work as a fix for the array_merge_recursive()
approach mentioned above in case anyone else finds it useful.
But I'd still love to hear from maintainers or anyone else who knows if there is a better way to solve this.
Updating to the latest 7.x-2.x-dev.
I couldn't easily generate an interdiff because the old patch no longer applied. There were only two main differences:
1) The patch in #79 contained a change from xmlsitemap_get_changefreq_options()
to xmlsitemap_get_changefreq_values()
, however a few other places in the module were referencing the _options()
function, so I excluded that change from the patch.
2) The previous comment had some @file comments which differ from the current branch, and they don't need to be a part of this patch.
hargobind → created an issue.
Adding a new patch that applies against 7.x-2.x-dev based on #10.
Attached is a simple patch to fix this.
hargobind → created an issue.
Thank you @alena_stanul for your deep analysis of this.
Although this issue is more about documentation of code, rather than the code itself, it's still helpful to get things right.
I looked into this by following your logic, and the patch in #2 checks out.
Much appreciated @paulocs!
Came here to say that it would be great to see this release soon as I'm transitioning a handful of sites to PHP 8.1, and it would be nice to get a full release into our deploy process.
Improved the patch by making sure $filters is an array.
RTBC++
Upgrading the priority to Major so a new release can be created, since lots of sites will start shifting to PHP 8.1 now that older versions are EOL.
Attached is a patch that fixes these three lines.
hargobind → created an issue.
The patch in #2 no longer applies against 7.x-2.x-dev. Attaching an updated patch.
I'm still testing this myself to see if it fixes the problem. I'll report back if I can.
Attached is a patch that changes how scald_galleria_get_library_file()
searches the library directory to find *.js paths.
Assuming the library path of sites/all/libraries/galleria, this new code searches the following directories in this order:
sites/all/libraries/galleria
sites/all/libraries/galleria/dist
sites/all/libraries/galleria/src
There is also a change to the filenames passed to glob()
, i.e. galleria*.js
instead of the old galleria-*.js
. Older versions of the library had filenames like "galleria-1.4.2.js", but all versions that are currently available on github.com simply use "galleria.js" or "galleria.min.js".
hargobind → created an issue.
Attached is a patch that checks if $original_path
is NULL and returns immediately.
hargobind → created an issue.
Readding the D7 patch that was hidden by @Lekstat
@Lekstat why did you hide your own patch in #79?