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

Bug fixes to RestfulDataProviderDbQuery #540

Closed
wants to merge 6 commits into from

Conversation

onumossn
Copy link

@onumossn onumossn commented Jul 5, 2015

As the view function was not calling getQuery, it would not include properties from joins that someone may do. If changes were made, it would be exactly the same as viewMultiple other than one accepts arrays, so I just called viewMultiple.

As a value was required to be not falsy for process_callbacks to be called, it would not allow processing falsy values. For example, certain tables in Drupal appear to use "1" and "0" for true and false values. Someone may want to convert those "1" and "0" to true and false before returning the data, but "0" is falsy and process_callback could not be called.

@e0ipso
Copy link
Member

e0ipso commented Jul 6, 2015

Thanks @mohammed-nauage-gsa can you see why tests are failing?

@onumossn
Copy link
Author

onumossn commented Jul 6, 2015

Fixed the tests. I am sorry about that. I had a hard time trying to setup testing locally. I am pretty new to php and drupal. Do things look ok now?

@@ -581,7 +554,7 @@ public function mapDbRowToPublicFields($row) {
}

// Execute the process callbacks.
if ($value && $info['process_callbacks']) {
if ($info['process_callbacks']) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should check !is_null()? I don't think we should process NULL values

@onumossn
Copy link
Author

onumossn commented Jul 6, 2015

I think its debatable, but I think that makes sense in most cases that I can imagine, so I updated it with isset since !is_null === isset.

Also, I noticed viewMultiple doesn't ever seem to actually be used beyond how I used it in the this file. What do you think of removing it or should it stay to make sure a defined coding/naming pattern is kept?

@onumossn
Copy link
Author

Are there any other issues that need to be resolved with this pull requests?

@e0ipso
Copy link
Member

e0ipso commented Aug 16, 2015

@mohammed-nauage-gsa sorry for the delay. I have been working intensely in the 2.x branch.

e0ipso pushed a commit to e0ipso/restful that referenced this pull request Aug 16, 2015
Refactor view code to use viewMultiple so it can leverage some fixes that
happened along the way in that method.
@e0ipso
Copy link
Member

e0ipso commented Aug 16, 2015

Merged.

Thanks again!

@e0ipso e0ipso closed this Aug 16, 2015
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.

3 participants