Austin, Texas
Account created on 15 December 2007, almost 17 years ago
  • Owner/Lead developer at 3Sages 
#

Merge Requests

Recent comments

🇺🇸United States hargobind Austin, Texas

Great job. Thanks for your work on this!

🇺🇸United States hargobind Austin, Texas

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.

🇺🇸United States hargobind Austin, Texas

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.

🇺🇸United States hargobind Austin, Texas

Per @Input's comment in #34, attached is a patch which makes weight configurable.

This patch is for 7.x.

🇺🇸United States hargobind Austin, Texas

hargobind changed the visibility of the branch 7.x-2.x to hidden.

🇺🇸United States hargobind Austin, Texas

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

🇺🇸United States hargobind Austin, Texas

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

🇺🇸United States hargobind Austin, Texas

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?

🇺🇸United States hargobind Austin, Texas

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

🇺🇸United States hargobind Austin, Texas

hargobind changed the visibility of the branch 3460676-php-8.0-deprecate to hidden.

🇺🇸United States hargobind Austin, Texas

The error above applies to the 7.x-1.x branch.

🇺🇸United States hargobind Austin, Texas

Attaching interdiff from #33 to #36.

🇺🇸United States hargobind Austin, Texas

Similar to #35, I experienced the same error when running on PHP 8.x.

Attached is an updated patch to fix this.

🇺🇸United States hargobind Austin, Texas

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.

🇺🇸United States hargobind Austin, Texas

Confirming that #2 fixes the issue.

The issue was reproducible for me during site cache clear.

🇺🇸United States hargobind Austin, Texas

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

🇺🇸United States hargobind Austin, Texas

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.

🇺🇸United States hargobind Austin, Texas

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.

🇺🇸United States hargobind Austin, Texas

Fix for accidental overwriting of string in last patch.

🇺🇸United States hargobind Austin, Texas

Attached patch uses the proposed resolution.

🇺🇸United States hargobind Austin, Texas

hargobind created an issue.

🇺🇸United States hargobind Austin, Texas

Ohh right, of course ;-)
Thanks for fixing this.

🇺🇸United States hargobind Austin, Texas

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?

🇺🇸United States hargobind Austin, Texas

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.

🇺🇸United States hargobind Austin, Texas

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.

🇺🇸United States hargobind Austin, Texas

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.

🇺🇸United States hargobind Austin, Texas

The patch in #2 works perfectly (even 8 years later).

The patch still applies cleanly to the 7.x-1.x branch as-is.

🇺🇸United States hargobind Austin, Texas

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:

  1. 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.
  2. 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.
  3. 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
  4. 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)
  5. It has already been mentioned that this module is unsupported. So adding new complexity will make it harder to get a patch in.
  6. 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).

🇺🇸United States hargobind Austin, Texas

Wonderful. Thank you all!

🇺🇸United States hargobind Austin, Texas

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

🇺🇸United States hargobind Austin, Texas

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.

🇺🇸United States hargobind Austin, Texas

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?

🇺🇸United States hargobind Austin, Texas

Accidentally included the ['user_list'] in the previous patch.

🇺🇸United States hargobind Austin, Texas

Attaching a patch with this one-line change.

🇺🇸United States hargobind Austin, Texas

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

🇺🇸United States hargobind Austin, Texas

hargobind created an issue.

🇺🇸United States hargobind Austin, Texas

Fix to #84 to rename the new options function to xmlsitemap_get_changefreq_form_options().

🇺🇸United States hargobind Austin, Texas

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.

🇺🇸United States hargobind Austin, Texas

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.

🇺🇸United States hargobind Austin, Texas

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.

🇺🇸United States hargobind Austin, Texas

hargobind created an issue.

🇺🇸United States hargobind Austin, Texas

Adding a new patch that applies against 7.x-2.x-dev based on #10.

🇺🇸United States hargobind Austin, Texas

Attached is a simple patch to fix this.

🇺🇸United States hargobind Austin, Texas

hargobind created an issue.

🇺🇸United States hargobind Austin, Texas

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.

🇺🇸United States hargobind Austin, Texas

Much appreciated @paulocs!

🇺🇸United States hargobind Austin, Texas

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.

🇺🇸United States hargobind Austin, Texas

Improved the patch by making sure $filters is an array.

🇺🇸United States hargobind Austin, Texas

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.

🇺🇸United States hargobind Austin, Texas

Attached is a patch that fixes these three lines.

🇺🇸United States hargobind Austin, Texas

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.

🇺🇸United States hargobind Austin, Texas

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

🇺🇸United States hargobind Austin, Texas

Any chance you could commit the 7.x patch from #8?

🇺🇸United States hargobind Austin, Texas

Attached is a patch that checks if $original_path is NULL and returns immediately.

🇺🇸United States hargobind Austin, Texas

Readding the D7 patch that was hidden by @Lekstat

@Lekstat why did you hide your own patch in #79?

Production build 0.71.5 2024