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

Add support for TimberPost->terms( array ) & more #737

Merged
merged 5 commits into from Nov 10, 2015

Conversation

Projects
None yet
2 participants
@aaemnnosttv
Copy link
Contributor

commented Nov 8, 2015

This PR addresses a few things (sorry about that) but I tried to limit the reach of the code changed to only that which is relevant. I think the commit messages give a pretty good summary, but here's the gist of it:

  • Add tests & support for calling the terms/get_terms methods on a TimberPost and passing an array of taxonomies to fetch terms for, rather than all of them.

It seems as though support for this was intended, but not documented, or tested for, and was not working as such.

  • Refactor existing tests for terms as well as tags and categories to be cleaner and more readable
  • Add a static named constructor for TimberTerm, which takes the same arguments as the constructor method. Usage like TimberTerm::from('My Term Name'). This is also totally compatible with extended classes. MyExtendedTimberTerm::from('My Term Name')
  • Remove usage of _get_terms property and deprecate it on TimberPost. This cache is not only unnecessary, but could actually return the wrong results as it only caches based on the taxonomy argument, and did not take the other method arguments into consideration.

aaemnnosttv added some commits Nov 8, 2015

move default TermClass to a property, similar to TimberPost
This is backwards compatible as if a TermClass is passed to the method it will take precedence over the property
refactor get_terms method to support an array of taxonomies
update docblocks.

also removes usage of _get_terms property, as this could return wrong results depending on arguments passed to the method
@jarednova

This comment has been minimized.

Copy link
Member

commented Nov 9, 2015

@aaemnnosttv this is great work. Thanks much for your work on this (and with tests!). I had a question on the TimberTerm::from('My Term Name') — can you provide an example of how/where you would use this in an example scenario (is this for usage in PHP or Twig or both?) thanks!

@aaemnnosttv

This comment has been minimized.

Copy link
Contributor Author

commented Nov 10, 2015

Thanks Jared! I've really been digging Timber, so I'm happy to contribute.

Regarding the named constructor, I actually use this in the tests. In addition to just being maybe a bit more expressive than using new, the static method makes it possible to use as a callback. Perhaps the best example might be using it as a callback for transforming an array of terms, into TimberTerms.

// some function/method..
// get all non-empty terms for categories and tags
$terms = get_terms(['post_tag','category']);

return array_map('TimberTerm::from', $terms);

Whereas without that, you would need to use a closure or something to map over that in order to new TimberTerm..

Tests are another reason (and probably the reason I thought to add it) where you can see above, we're able to convert all of those array items to TimberTerms without using a foreach, which would unnecessarily pollute the test method with variables that were really not necessary.

It's just a little helper which can be used to help with readability, but also with reducing the amount of code needed to do certain things. :)

@jarednova

This comment has been minimized.

Copy link
Member

commented Nov 10, 2015

Awesome, something I hadn't thought of before. Would it make sense to apply the same method to posts and users as well?

jarednova added a commit that referenced this pull request Nov 10, 2015

Merge pull request #737 from aaemnnosttv/pr/timber-terms-with-array
Add support for TimberPost->terms( array ) & more

@jarednova jarednova merged commit f72c16f into timber:master Nov 10, 2015

3 checks passed

Scrutinizer 15 new issues, 2 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 83.675%
Details
@aaemnnosttv

This comment has been minimized.

Copy link
Contributor Author

commented Nov 10, 2015

Yeah I think the same use case would apply to all of them. I definitely thought of extending that to those as well, but as I said, I wanted to keep this PR limited ;)

I could put together another one if you're interested?

@jarednova

This comment has been minimized.

Copy link
Member

commented Nov 10, 2015

@aaemnnosttv yes please! Thanks for making it a separate PR though, one of the toughest things can be wading into code that touches too many pieces at-once.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.