Skip to content
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

CRM-21300: Addressed notices due to unexpected NULL from Joomla DB object. #11135

Merged

Conversation

universalhandle
Copy link
Contributor

@universalhandle universalhandle commented Oct 13, 2017

Overview

Prevent notices described in CRM-21300 from being raised.

Before

See CRM-21300.

After

No more notices or warnings (needs validation).

Technical Details

The Joomla DB object returns NULL in some cases, which was not immediately obvious from the documentation. This PR ensures that the DBO-wrapping method always has a consistent return type.


@universalhandle
Copy link
Contributor Author

@totten, I haven't actually run this code, but it should address the warnings/notices you reported, all of which seem to follow on the original unexpected return type. Could you please validate?

@seamuslee001
Copy link
Contributor

@mattwire can you take a look at this and see if this would change anything on your joomla site?

@eileenmcnaughton eileenmcnaughton changed the base branch from master to 4.7.27-rc October 16, 2017 04:35
@eileenmcnaughton eileenmcnaughton changed the base branch from 4.7.27-rc to master October 16, 2017 04:35
@eileenmcnaughton
Copy link
Contributor

@GinkgoFJG can you put this against 4.7.27rc- as a recent regression this should go in the rc

@eileenmcnaughton
Copy link
Contributor

Also the calling function looks like

    $associations = $this->getUserGroupPermsAssociations();
    $cmsPermsHaveGoneStale = FALSE;
    foreach (array_keys(get_object_vars($associations)) as $permName) {

so we shouldn't return an array if null unless we also return an array if successful (which is probably the cleanest - ie. pass TRUE as the second param to json_decode

@universalhandle
Copy link
Contributor Author

We are not returning an array in the case of a failed query; we are returning an object with no properties. The json_decode() call returns an object of type stdClass (because that is how Joomla stores the permissions data). I think this is cleanest/safest because:

  • with this change, getUserGroupPermsAssociations() returns a stdClass object no matter what
  • get_object_vars($associations) will return an empty array in the case of a failed query, effectively short-circuiting the foreach loop

@universalhandle universalhandle changed the base branch from master to 4.7.27-rc October 16, 2017 14:00
@universalhandle universalhandle changed the base branch from 4.7.27-rc to master October 16, 2017 14:00
@universalhandle universalhandle changed the base branch from master to 4.7.27-rc October 16, 2017 14:08
@universalhandle
Copy link
Contributor Author

@eileenmcnaughton: I've rebased against 4.7.27-rc and changed the base of the PR.

@mattwire
Copy link
Contributor

@GinkgoFJG @seamuslee001 I'll try out later on a Joomla 3.8.1 site. Haven't seen any warnings/notices though so I guess I'm just looking for "it still seems to work"!

@mattwire
Copy link
Contributor

Ok no adverse affects - seems ok to me!

@eileenmcnaughton
Copy link
Contributor

test this please

@seamuslee001
Copy link
Contributor

This looks good to me. Code looks safe, this has passed Jenkins and has been tested by Matt on a Joomla install with no errors showing up. I think this is good to merge. Note i haven't personally tested the code but it make sense to me and agree with Franks's comments re the stdObject

@totten
Copy link
Member

totten commented Oct 17, 2017

Nice, @GinkgoFJG !

There are multiple supportive comments. Also, I can confirm that this resolves the warnings on my workstation when installing from an extracted .zip on Joomla 3.6.

Merging.

@totten totten merged commit 8ca9fa9 into civicrm:4.7.27-rc Oct 17, 2017
@universalhandle universalhandle deleted the CRM-21300-joomla-perms-notices branch October 17, 2017 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants