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

Accept an array of properties in get/getLiteral/getResource methods #326

Merged
merged 3 commits into from Aug 27, 2020

Conversation

osma
Copy link
Contributor

@osma osma commented Jun 12, 2020

This PR changes the implementation of EasyRdf\Graph::get to accept an array of properties. Previously the implementation was inconsistent with the phpdoc declarations: the phpdocs for getLiteral and getResource methods in both Graph and Resource classes state that it is possible to pass an array of properties, but it didn't actually work (see #325). Also the phpdoc description for Graph::get hints that it is possible to pass an array.

The PR straightens out this wrinkle so that it is actually possible to pass an array to all the above mentioned methods. The actual fix was simple - only two new lines of code - but then I had to change the phpdoc a little to reflect that, and also the exception message that complains about bad values now makes it clear that an array is accepted, too.

Finally I had to do small changes to existing unit tests that check for the exception message, plus I added two more to verify that passing an array of properties actually works as intended.

Fixes #325, ping @k00ni

@k00ni
Copy link
Contributor

k00ni commented Jul 6, 2020

👍 for merging it

@k00ni
Copy link
Contributor

k00ni commented Aug 13, 2020

@njh and @zozlak, any objections? @indeyets approved it.

@zozlak
Copy link
Collaborator

zozlak commented Aug 13, 2020

I'm pretty sure the fix won't work for fully-qualified URIs (e.g. $resource->get(['https://foo/test', 'https://:foo/bar']);.

But it's a part of a more general issue of accepting a property-like parameter of a mixed type and the way it's handled internally in the EasyRdf.

@zozlak
Copy link
Collaborator

zozlak commented Aug 13, 2020

By the way, it will probably work for $resource->get(['<https://foo/test>', '<https://:foo/bar>']); but this should be stated in the documentation.

And one more scenario to be tested is $resource->get([$graph->resource('https://foo/test'), $graph->resource('https://foo/test')]);

Copy link
Contributor

@k00ni k00ni left a comment

Choose a reason for hiding this comment

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

👍

Any objections @njh?

Copy link
Collaborator

@njh njh left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@njh njh merged commit 3018991 into easyrdf:master Aug 27, 2020
@njh njh added this to the 1.1.0 milestone Aug 28, 2020
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.

Can't give array of properties to getLiteral or getResource
5 participants