Skip to content

Conversation

@aaemnnosttv
Copy link
Contributor

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.

This is backwards compatible as if a TermClass is passed to the method it will take precedence over the property
update docblocks.

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

@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
Copy link
Contributor Author

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
Copy link
Member

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
Add support for TimberPost->terms( array ) & more
@jarednova jarednova merged commit f72c16f into timber:master Nov 10, 2015
@aaemnnosttv
Copy link
Contributor Author

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
Copy link
Member

@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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants