Skip to content

CRM-20419 jQuery.isEmptyObject misused #10152

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 4, 2017
Merged

Conversation

agh1
Copy link
Contributor

@agh1 agh1 commented Apr 12, 2017

This fixes a bunch of places where $.isEmptyObject() is used on arrays and strings. The method's documentation is clear that it may yield inconsistent results when used on anything other than a basic object. In practice, at least some Joomla sites have had problems where CRM.$.isEmptyObject() returns false on an empty array or string despite returning true in most other environments.

This replaces all instances of $.isEmptyObject() in the codebase aside from the couple that actually evaluate objects.


@eileenmcnaughton
Copy link
Contributor

@colemanw one for you to check?

@lcdservices
Copy link
Contributor

Tested this PR and confirmed it fixes the problem.

@eileenmcnaughton
Copy link
Contributor

@lcdservices do you understand the code well enough to say that it is also 'safe' to merge? I'm not familiar with it :-(

@lcdservices
Copy link
Contributor

I did a little reading and using .length seems to be an acceptable (and sometimes recommended) alternative to .isEmptyObject() to avoid the strict nature of the latter. so I think this is safe to merge.

@colemanw colemanw merged commit 58bf7ae into civicrm:master May 4, 2017
@seamuslee001
Copy link
Contributor

Just noting this is causing a regression on dmaster http://dmaster.demo.civicrm.org/civicrm/admin/contribute/custom?reset=1&action=update&id=1

@colemanw
Copy link
Member

Thanks @seamuslee001 - I've submitted #10331

@agh1
Copy link
Contributor Author

agh1 commented May 11, 2017

Thanks @seamuslee001 and @colemanw!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants