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

prevent throw array access error when orderByChild('nested/key') #135

Closed
wants to merge 2 commits into from

Conversation

llstarscreamll
Copy link
Contributor

This PR prevent throw array access errors when use nested Firebase paths on orderByChild. Look at the next query example:

$firebase->getReference('users')
  ->orderByChild('profile/email') // nested Firebase path
  ->startAt('john@')
  ->getValue();

The schema:

"users": {
  "a1": {
    "profile": {
      "email": "[email protected]"
     }
  }
}

Before this throwed [ErrorException] Undefined index: profile/email.

Now this query doesn't throw exceptions.

@jeromegamez
Copy link
Member

Thank you for the contribution! I didn't even know it was possible to order by a nested child value - is it? And if so, why isn't it documented in the official docs 😅?

I will have a deeper look at it shortly - I will certainly rework the helper method, because I think it could be structured a lot nicer (and code climate agrees 😅), but I will make sure that your commits are preserved.

@llstarscreamll
Copy link
Contributor Author

Hi @jeromegamez, I have been very busy these days, I made a small cleaning up and renamed the helper method, all is green now, make the modifications you consider to this PR.

Thank you so much for this library!!

Cheers!!

@jeromegamez
Copy link
Member

Thank you Johan! The reason why it's taking me so long to review and integrate this feature is that I have to understand what's happening and to be sure that it's the best and easiest possible way to provide a feature (as it's me who has to provide support for the library in the future :)).

I do understand what's happening and why this is needed, but my gut tells me that there's an easier way to do it ^^. Your PR will be merged and you'll get attributed for the feature, please allow me a little bit more time for it :). Cheers!

@jeromegamez
Copy link
Member

Hey Johan! Thanks again for your contribution - please have a look at Release 3.7.0 - it includes your contribution and the changes I made based upon your work:

  • Added tests to ensure that everything is working as expected (it did with your solution)
  • Used Jmespath to check for the nested values - the library is already included in the SDK and provides a similar functionality to the one you provided.

Cheers!

@jeromegamez jeromegamez closed this Dec 8, 2017
@llstarscreamll
Copy link
Contributor Author

Great!! Thank you so much!!

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

Successfully merging this pull request may close these issues.

2 participants