Continue updating deprecated jQuery 3.7 methods

[argiepiano]

This is a follow-up to https://github.com/backdrop/backdrop-issues/issues/6211.

There are still a few methods that have been deprecated, that need to be replaced. These are some of those:

• jQuery.fn.bind(), jQuery.fn.unbind()
• jQuery.trim (in views_admin.js, backdrop.js, jquery.treetable.js, user.admin.js and in ajax.js)
• jQuery.isArray (in ajax.js)
• jQuery.isFunction (in ajax.js).

Additionally the date module uses a minified version of the jQuery Time Entry plug in, which is very old and contains some deprecated jQuery methods. The last release of that plugin seems 2014!

[klonos]

@argiepiano I've created #6249 for the jQuery Time Entry plugin.

[indigoxela]

So, let's start to work on that task. :wink:

That's what I found in our code:

$.parseJSON(
misc/ajax.js

$.trim(
misc/backdrop.js
modules/views_ui/js/views-admin.js
modules/user/js/user.admin.js

Possibly still not all... but doesn't look that bad. Not sure, when / where that code runs, unfortunately.

[indigoxela]

A PR is available now for testing.

Steps for user.admin.js and views-admin.js are as such:

user.admin.js

1. Add a language
2. admin/config/regional/settings - make sure "Users may set their own time zone." is checked
3. Go to user/XX/edit
4. In vertical tab "Region and language" toggle settings

views-admin.js

1. Go to admin/structure/views/view/node_admin_content
2. Section "Fields", click on ADD
3. Click on any checkbox in the list

[indigoxela]

Re ajax.js and backdrop.js

I have no idea, when that part of ajax.js ever runs, as response always seems to be an array. But then I also have no idea, what "iFrame uploads" are supposed to be. :shrug:

Also no clue re the part in backdrop.js - how to trigger an error, so this runs... :shrug:

[argiepiano]

I have no idea, when that part of ajax.js ever runs, as response always seems to be an array. But then I also have no idea, what "iFrame uploads" are supposed to be

I believe (and I may be wrong!) that some contrib modules use iframes to upload files, in which case the response is a json encoded string. See imce.

[laryn]

I did a quick search for iframe in the imce repo and all that comes up is this commit from 2014:

Updated jquery.form.js: Got rid of iframe upload and html response for modern browsers that support FormData object.

[argiepiano]

I see one instance of iframe here, but that may be something else. No time to look into it now...

[laryn]

I stand corrected, that is now showing up for me as well. Maybe the repo wasn't fully indexed...? I'm also not sure if that relates to uploading, I'm not that familiar with using imce.

[indigoxela]

Re the iframe in IMCE - that's the file browser tab for users on user/X/imce, but is unrelated to file upload. Even when uploading with IMCE, the typeof response is an array.

The most plausible explanation so far seems to be, that this is some leftover code from a polyfill for old IEs. Possibly dead code for quite a while. But just in case, some contrib or custom implementations still rely on it (?), we might keep it in place and just fix it "on spec". What do you think?

[klonos]

Thanks @indigoxela 🙏🏼 ...the code changes look good to me 👍🏼

I'm making this an on-going issue. Lets keep iterating over smaller chunks of gradual cleanup/fixing instead of trying to catch everything in one go.

Wondering if it would help if we tried to add these functions/deprecations as forbidden regex in our CSpell checks? (to avoid re-introducing deprecated code in our codebase) ...I've done some light research on whether there's any ready-made tool to catch jQuery deprecations, but was unable to find one ...well, there are some, but they seem to be catching all jQuery functions that have native, pure JS equivalents (which is not what we want). Perhaps after we've progressed our efforts in #6402 we might be able to find an efficient way to do this. (correction: there are 3rd party ESLint plugins that allow custimization after all - see #6402 for more details, but the README of https://github.com/wikimedia/eslint-plugin-no-jquery mentions the follwoing:

The config plugin:no-jquery/deprecated includes all known deprecated and removed code, and is updated as new releases of jQuery come out. You can instead use configs targeting specific versions of jQuery if you know the environment in which your code will operate. There is a config for all minor versions from 1.0 to 3.6 (deprecated-1.0, ..., deprecated-3.7). Deprecation configs are cumulative, so they include all the rules for jQuery versions below them.

[indigoxela]

I'm making this an on-going issue.

We're (hopefully) done now. :wink: Unless you'd like to keep this issue open for future jQuery deprecations (5, 6...). Note that there's still the problem with 3rd party plugins, find more details in #6404.

Wondering if it would help if we tried to add these functions/deprecations as forbidden regex in our CSpell checks?

That's an interesting idea, but I'm not sure if that wouldn't be an abuse of spell-checking. :thinking:

@klonos the jquery-migrate project aims to catch deprecated functionality. But it's not supposed to be on all the time, it's more like a development tool. In fact, I used that locally, to find most of the deprecated stuff in core in a previous issue.

[avpaderno]

To avoid some functions are called, PHP_CodeSniffer has rules for that. For example, Coder uses the following lines in its phpcs.xml.dist file.

<rule ref="Generic.PHP.ForbiddenFunctions">
    <properties>
        <property name="forbiddenFunctions" type="array">
            <element key="sizeof" value="count"/>
            <element key="delete" value="unset"/>
            <element key="print" value="echo"/>
            <element key="is_null" value="null"/>
            <element key="create_function" value="null"/>
        </property>
    </properties>
</rule>

They even allow to suggest an alternative function (count() instead of sizeof()).

[indigoxela]

To avoid some functions are called, PHP_CodeSniffer has rules for that.

@kiamlaluno well, they used to have sniffs for js, but completely dropped them. Coder uses a dry-aged version of phpcs, our core check and newly created ruleset uses a recent one (3.x).

[avpaderno]

@indigoxela Whoops, I read functions and I thought to PHP functions. Anyway, I agree that using CSpell to give a warning for functions that should not be called is not how CSpell is supposed to be used.

[indigoxela]

Funny... or not. :shrug:

This issue turns out to actually be "ongoing", because a recent commit to core introduces a deprecated function in core/misc/ajax.js

And it does it in a way, that seems related to security, so I can't simply wipe that $.parseJSON out, as I normally would.

@quicksketch can we find a different approach for that check? Or do we have to live with this deprecation?

In jQuery 4 this function will get removed (which is when this jquery.form code will get dangerous), but there will be a BC layer - see #6404 for details. We might be able to introduce that layer in a way, that already catches things like that for jQuery 3 - by introducing that wrapper around JSON.parse earlier.

At least, I do have questions re that recent commit. :wink:

[indigoxela]

Time for a rebase here - done. :wink:

[quicksketch]

@quicksketch can we find a different approach for that check? Or do we have to live with this deprecation?

The safety check that was added does not actually use a deprecated function, it actually defines it if it does not exist:

    if (!$.parseJSON) {
      $.parseJSON = JSON.parse;
    }

But my understanding as to why we added this is because certain earlier versions of jQuery did not have this function, thus causing the security vulnerability to be exploitable. However, Backdrop has never included one of these early versions of jQuery. We added it at the time because it was seen as a more comprehensive fix, even if it was likely unnecessary.

However, we did not know at the time that $.parseJSON was deprecated and would be removed in newer versions of jQuery. So ironically, jQuery 4.x (when we add it) will make the jquery.forms.js library vulnerable again, because this method will no longer be defined.

I think that means for the time being, we should leave the check in place. We can remove it if we decide to separately maintain a fork of jquery.forms.js, and remove the vulnerability from that library.

Anyway, overall for the current PR. Looks good! I think this is ready to commit.

[indigoxela]

jQuery 4.x (when we add it) will make the jquery.forms.js library vulnerable again,...

We have an issue for v4, and a PR, that adds a compatibility layer, which prevents exactly that (and other problems).

I think that means for the time being, we should leave the check in place.

Sure, it just looked odd and hackish (only) at first sight.

[klonos]

Do we have inline comments explaining that that is a security thing that should not be removed? We should!

[quicksketch]

I went ahead and merged https://github.com/backdrop/backdrop/pull/4656 into 1.x and 1.28.x.

@klonos We usually don't add comments around security fixes (at least initially). It's sort of weird practice that perhaps might not be necessary, but the code changes and commit message are usually intentionally a bit vague, to make it so that it's not easy to find security issues in an automated way or to indicate how they might be exploited.

Long-term, our goal here should be to eliminate the work-around entirely, either by forking jquery.forms.js so the work-around is not needed, or stop using jquery.forms.js.

[klonos]

@indigoxela are there any other sub-tasks remaining to be done as part of this on-going task?

[indigoxela]

are there any other sub-tasks remaining to be done as part of this on-going task?

@klonos None I'm aware of, no.