Skip to content

Feature/interfaced object factories#1220

Closed
joshlevinson wants to merge 10 commits intotimber:masterfrom
joshlevinson:feature/interfaced-object-factories
Closed

Feature/interfaced object factories#1220
joshlevinson wants to merge 10 commits intotimber:masterfrom
joshlevinson:feature/interfaced-object-factories

Conversation

@joshlevinson
Copy link
Copy Markdown

This builds on #1218 and #1219 to provide interfaced object factories, separate for each object type.
This pattern allows developers to instantiate their own Factory instances, with which they can specify the "default" class to be used.
This PR also updates Timber to use these factories; there are several instances where the factories are directly instantiated over using the static get method for instantiation & object retrieval in one go.

This style provides different factories for Posts vs Terms, and allows direct instantiation of the factory vs strictly a static method.
This is more future proof than the purely static method of timber#1218, and opens the door for stubs and better testing.
@joshlevinson
Copy link
Copy Markdown
Author

joshlevinson commented Oct 24, 2016

It's possible that the \Timber\Factory\*Getter classes could be combined with the \Timber\*Getter classes. There seems to be some duplicate logic between them.
@jarednova Do you know if the \Timber\*Getter classes are used by 3rd parties?

@joshlevinson
Copy link
Copy Markdown
Author

FWIW, this is how I'm overriding Timber's class map to provide my own classes:

<?php

namespace MyProject;

add_filter( 'Timber\TermClassMap', function ( $classes ) {
    return add_timber_classes( $classes, 'term', get_taxonomies() );
} );

add_filter( 'Timber\PostClassMap', function ( $classes ) {
    return add_timber_classes( $classes, 'post', get_post_types( [ 'public' => true ] ) );
} );

/**
 * @param $class
 * @param $default_type
 * @param $all_types
 *
 * @return array
 */
function add_timber_classes( $class, $default_type, $all_types ) {
    $class = (array) $class;

    $default = slugs_to_fq_class_name( '\\ObjectTypes\\' . slugs_to_camel_case( $default_type ), $default_type );

    foreach ( $all_types as $type ) {
        $class_name = slugs_to_fq_class_name( '\\ObjectTypes\\' . slugs_to_camel_case( $default_type ), $type );
        if ( class_exists( $class_name ) ) {
            $class[ $type ] = $class_name;
        } else {
            $class[ $type ] = $default;
        }
    }

    return $class;
}

/**
 * @param $slug
 *
 * @return mixed|string
 */
function slugs_to_camel_case( $slug ) {
    $slug = str_replace( [ '-', '_' ], ' ', $slug );
    $slug = ucwords( $slug );
    $slug = str_replace( ' ', '', $slug );

    return $slug;
}

/**
 * @param $type
 * @param $slug
 *
 * @return string
 */
function slugs_to_fq_class_name( $type, $slug ) {
    return __NAMESPACE__ . slugs_to_camel_case( $type ) . '\\' . slugs_to_camel_case( $slug );
}

With this setup, I can create a default structure (namespace & file) like:

\ObjectTypes\Post\Post.php
\ObjectTypes\Term\Term.php

and then specify more depth like:
\ObjectTypes\Post\MyPostType.php
\ObjectTypes\Post\MyTaxonomy.php

…edence

Previously, the function argument for the class was only considered if the Class Map filter didn't yield a valid class
This changes the logic so that if a class is specified, it takes priority
The final is still filterable via `Timber\[Post|Category]ClassMap`
jarednova added a commit that referenced this pull request Nov 5, 2016
@coveralls
Copy link
Copy Markdown

coveralls commented Nov 7, 2016

Coverage Status

Coverage decreased (-2.9%) to 92.392% when pulling 673dca4 on joshlevinson:feature/interfaced-object-factories into 3a1deee on timber:master.

@jarednova
Copy link
Copy Markdown
Member

Hey @joshlevinson!

I took a spin through the PRs this weekend and have a bunch of questions. Where to start? Well, here it goes...

1: The PRs all seem to build on eachother, is that right? Where #1220 contains everything from #1219 and #1218

2: The code sample showing the class maps in #1219 is very helpful, can you provide some other key usage examples? Specifically it's helpful to see old vs. new that captures how the new syntax is cleaner/clearer/better. Next step: Can you itemize the key differences in API and then go from there in creating examples?

3: Backwards compatibility: I definitely want to push the code/platform forward (which might mean some old things break) but I want to preserve the general allowed ways to interact with Timber (for example, being able to declare a new TimberPost();) unless it's going to totally bork us going forward. I think this really helps users transition to a better way rather than them throwing their hands-up in frustration. Next step: Check out this branch where I've restored some default args (and commented-out the doing_it_wrongs so they don't throw warnings)

Let me know what you think on those points — is it cool if I add you as an official collaborator? It'll make things much easier for us to go back-and-forth on code changes.

@joshlevinson
Copy link
Copy Markdown
Author

@jarednova,

1: The PRs all seem to build on eachother, is that right?

Yep!

2: ...Can you itemize the key differences in API and then go from there in creating examples?

The primary difference in use of Timber is whether or not an implementor should use new to create Timber objects. With factories, one should rely on the respective factory to instantiate the object for them, passing all necessary data for the construction of the object into the factory as they would in the constructor of the desired object. Instead of declaring:

$post = new \Timber\Post( 1 );
$term = new \Timber\Term( 2, 'category' );

With factories, one would do:

$post = \Timber\Factory\PostFactory::get( 1 );
$term = \Timber\Factory\TermFactory::get( 1, 'category' );

I'll take a look at some other Timber codebases to get a better understanding of how Timber's classes are implemented, but by and large, my primary use of these factories has been using them to override the classes used in Timber's own instantiations (e.g. the main queries, in menus, etc).

3: Backwards compatibility

I'm totally with you on the BC reasoning. Declaring new Timber\Post will (in the current iteration of this PR) run the constructor arguments through the PostFactory, achieveing the same effect while preserving BC.
Note that this PR is limited to Post and Term factories; I could see adding more for other Timber classes that lack factories (e.g.Image).

Beyond documentation, I'd also like to consider how I can increase test coverage (or at least get it back to its level prior to these changes).

@jarednova
Copy link
Copy Markdown
Member

Doing some cleanup with @acobster deep into Factories over on this branch:

https://github.com/timber/timber/tree/2.x-factories

... closing @joshlevinson's greatly appreciated PR here. Even if the exact code won't live on, the spirit will. Thanks for getting us started down this path!

@jarednova jarednova closed this Jan 8, 2020
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