2.x Deprecate Timber\PostClassMap filter and rename it to timber/post/classmap

[gchtr]

Ticket: #2073

This pull request handles the following todo in #2073.

• Deprecate Timber\PostClassMap filter and rename it to timber/post/classmap. Currently, it’s called timber/post/post_class in the 2.x branch.

The filter timber/post/post_class was added in the 2.x way earlier, so it’s safe to remove it.

Considerations

I guess were not exactly deprecating the Timber\PostClassMap filter, because it will not work the way it did before. We can also consider using Helper::doing_it_wrong() instead. On the other hand, when upgrading to 2.0, you will want to remove all notices that will appear, including the notices about deprecated hooks. So maybe it doesn’t really matter.

What do you think?

Testing

Not yet. Now added.

[jarednova]

@gchtr I don't think it's necessary to go too deep on the deprecation protections here (beyond what you've done already). The way classes are handled with Factories means so many changes that I don't think a single "missing" doing_it_wrong() is really going to be of much value.

Would you like me to develop tests here or do you already have that covered?

[jarednova]

Just need to make sure we have proper tests in place ...

[gchtr]

@gchtr I don't think it's necessary to go too deep on the deprecation protections here (beyond what you've done already). The way classes are handled with Factories means so many changes that I don't think a single "missing" doing_it_wrong() is really going to be of much value.

👍

Would you like me to develop tests here or do you already have that covered?

@jarednova Currently, you could only add tests for the deprecation of the filter, but not how the post class map actually works, because Coby is still working on that. But I’d be glad if you could add the tests for the deprecation.

[acobster]

The Class Map filters are all tested at the Factory layer. We don't have any doing_it_wrong() reporting right now because I wanted to circle back to it once we had a good architecture. That has been discussed here as well. I thought I remembered some follow-up discussion to that but I can't find it right now; in any case I don't think we arrived at a conclusion.

My main concern in this area is what happens when someone returns a class name from a timber/*/classmap filter that doesn't implement CoreInterface. PHP will throw a Fatal Error when the Factory's ::build() method tries to return it. So my questions is whether we call doing_it_wrong() and let it fail directly afterward, or if we throw our own exception immediately explaining exactly what happened.

[jarednova]

@acobster if I'm following you here correctly, I think the place to go is with ...

we throw our own exception immediately explaining exactly what happened.

Since it's a fatal error, an exception would feel most appropriate. In the docs for WordPress's _doing_it_wrong() it uses the phrase "called incorrectly" which feels like a softer warning.

[jarednova]

@gchtr for some reason this is triggering a ton of ...

[ Timber ] Class T either does not exist or implement \Timber\Post

warnings in PHPUnit. I'm trying to rope through and figure out where that's originating, but can't seem to track it down. Is this worth time? or does the forthcoming work from @acobster (perhaps) render that irrelevant?

[gchtr]

The Class Map filters are all tested at the Factory layer. We don't have any doing_it_wrong() reporting right now because I wanted to circle back to it once we had a good architecture. That has been discussed here as well. I thought I remembered some follow-up discussion to that but I can't find it right now; in any case I don't think we arrived at a conclusion.

Discussion is a bit scattered. I guess we discussed this in #2073 (comment) and then further in #2196, which led to this issue for the future: #2210.

I'm trying to rope through and figure out where that's originating, but can't seem to track it down. Is this worth time? or does the forthcoming work from @acobster (perhaps) render that irrelevant?

I would really wait for the rest of the post factories to be implemented first, and then come back to see how we do error handling. I only wanted to work alongside Coby. There are a few other pull requests that also need to wait for other functionality. I’m going to add wip labels for these to make it more clear.

[acobster]

FYI I updated the base branch here just to compare with the bleeding-edge Post API. We can still move forward with this (or not) however we choose.

@gchtr I think the TODOs for this PR boil down to:

• PostGetter will no longer be used at all, so the Timber\PostClassMap filter will have no effect at all. So if I'm understanding the way it worked in 1.x, there's nothing for us to really hook into and no way for us to even discern that a user is doing_it_wrong() by adding such a filter! So I think we should just make a note that this is a breaking change.
• It adds a docblock for the new timber/post/classmap filter.
• It (potentially) adds a check and a corresponding Exception when the result of the classmap filter is something other than a CoreInterface instance. We could write up a new issue for that, but at this point I'd favor just doing it here.

cc @jarednova

[gchtr]

I had to merge in the current version of the 2.x-posts-api branch.

PostGetter will no longer be used at all, so the Timber\PostClassMap filter will have no effect at all. So if I'm understanding the way it worked in 1.x, there's nothing for us to really hook into and no way for us to even discern that a user is doing_it_wrong() by adding such a filter! So I think we should just make a note that this is a breaking change.

Could we add a call to the deprecated filter right before we use the timber/post/classmap filter and check whether a filter was applied?

/**
 * Filters the class(es) used for different post types.
 *
 * @deprecated 2.0.0, use `timber/post/classmap`
 */
if ( 'deprecated' !== apply_filters( 'Timber\PostClassMap', 'deprecated' ) ) {
	\Timber\Helper::deprecated(
		'The `Timber\PostClassMap` filter', 
		'the `timber/post/classmap` filter',
		'2.0.0'
	);
}

// ...
$classmap = …

It adds a docblock for the new timber/post/classmap filter.

Should be okay now.

It (potentially) adds a check and a corresponding Exception when the result of the classmap filter is something other than a CoreInterface instance. We could write up a new issue for that, but at this point I'd favor just doing it here.

I’d be glad if you could add that!

[gchtr]

@acobster @jarednova I could still use some feedback on my recent comment, so we can finalize this 😊.

By merging in the current 2.x branch, we lost the PostGetter class, so apparently there’s no Timber\PostClassMap filter to deprecate. Still, I’d find it helpful if we could add a notice (not in the sense of a deprecation) in case this filter is still used.

[acobster]

@gchtr I see - just some extra guardrails around the new hooks, warning of the old usage. Makes sense! I will add that in along with the Exceptions, probably some time next week.

[gchtr]

@jarednova This is now good for another review.

I added my proposal from #2293 (comment) and added a test to make sure it works as expected. And I added the remaining todo as a separate issue in #2487 so that it doesn’t get lost.

[jarednova]

This looks good! (and feels like how it should have now always been). Thanks for pulling in some of those lingering notes and capturing #2487 for the future