2.x user factories by acobster · Pull Request #2160 · timber/timber

acobster · 2020-01-09T03:03:58Z

Ticket: #2073 #2088 #2094

Issue

Updating the Users API per discussion in #2073.

Solution

Extensive discussion of the solution is in #2073 and #2093. In short:

Timber::get_user and Timber::get_users() no longer take a $UserClass arg; users must now rely on the timber/user/classmap filter to achieve polymorphic instantiation.
The public Timber functions use Factories under the hood, which in turn apply timber/user/classmap.

Impact

Discussed in docs.

Usage Changes

Also discussed in the docs.

Considerations

I'm not sure what the new usage of the Twig {{ User(...) }} fn should be. Currently it supports arrays of IDs, which seems a bit astonishing. I'd be in favor or dropping support for this, or at least deprecating that use-case and providing a new {{ Users(...) }} (or {{ UserCollection(...) }}?) function.

Speaking of which, do we need a UserCollection class? I kind of assumed in the back of my mind that we'd want Collections for Users and Terms (we already have CommentThread), but thinking more critically about it, the Loop is the main reason we have PostCollection, and there's not really an analog to that with Users or Terms. I could see there being a use-case for pagination or some other advanced query-related stuff, but I'm not sure. Thoughts?

Edit: oh, and I'm hoping one of you can look at #2157 before we merge this in. :)

Testing

Tests have been refactored and removed where appropriate. The UserFactory class has some prety thorough tests as well. It would be great add to tests for any edge cases y'all can think of.

Like it says on the tin, this makes the User constructor protected. Also updates Post::author to use a Factory so class maps are supported when calling `Post::author()`.

we're dealing with a user object, not a post :P

#2153 add a test helper for auto-removing filters

gchtr · 2020-01-09T20:01:40Z

I'm not sure what the new usage of the Twig {{ User(...) }} fn should be. Currently it supports arrays of IDs, which seems a bit astonishing. I'd be in favor or dropping support for this, or at least deprecating that use-case and providing a new {{ Users(...) }} (or {{ UserCollection(...) }}?) function.

I think this was applied to match the behavior of other other types (post, terms, ...), without thinking about a use case.

But we will deprecate the Twig functions anyway and use the same function names that we use in PHP, see #2144. That would mean we should have a {{ get_users(...) }} function as well. The same goes for the other objects.

Speaking of which, do we need a UserCollection class? I kind of assumed in the back of my mind that we'd want Collections for Users and Terms (we already have CommentThread), but thinking more critically about it, the Loop is the main reason we have PostCollection, and there's not really an analog to that with Users or Terms. I could see there being a use-case for pagination or some other advanced query-related stuff, but I'm not sure. Thoughts?

We discussed this quite some time ago in #1689. My take is that we don’t need UserCollections, because the loop isn’t involved and usually we don’t need pagination. The same goes for Terms. In case we ever need that in the future, we can add it later.

I will have a thorough look at this later this week.

acobster · 2020-01-09T20:20:57Z

@gchtr ok, I remember that discussion now. I guess we can always add a ::get_users() option to return a UserCollection instead of an array if we decide to implement it. It also occurs to me that we should probably have a users.md among the Guides. Do you have the bandwidth to write one up?

gchtr · 2020-01-09T20:41:15Z

👍

It also occurs to me that we should probably have a users.md among the Guides . Do you have the bandwidth to write one up?

Yes, I will write one up 👍

gchtr

Omg, with factories, the code looks soo clean 🤩.

I added a first couple of comments.

I will come up with a separate pull request for additional inline documentation.

lib/Timber.php

lib/Factory/UserFactory.php

PHP below 7.2 does not support argument type declaration of `object`.

acobster · 2020-01-11T01:03:00Z

@gchtr I just got all unit tests passing for this locally.

Per discussion on #2157 I'm dropping support for Co-Authors Plus for now, as I just don't have time to continue supporting it. Discussion can continue on that PR, where the 2.x-coauthors-fixes branch contains all the code needed to re-add support for it.

How are you feeling about this PR?

… IDs

gchtr

This looks good to me!

I’m okay with dropping support for Co-Authors Plus and added a note in the Upgrade Guide in ffd474d. As you said, we can always come back later for trying to add support.
I added two more tests for the factory in 6995c6f.
I added a User Guide in #2167.
I added DocBlocks improvements for User Factories in #2168.

…such

…or InvalidUserClass

lib/Timber.php

jarednova · 2020-01-12T20:10:49Z

Extensive work here @acobster! I found a few tiny things, but overall this is looking super. Take a look and let me know if you agree with my one substantive change (returning false instead of null when no user found via Timber::get_user_by and other similar methods). This is the pattern I saw in CommentFactory and what would make the most logical sense to me*. Merge if you agree! (or we can discuss what makes the most sense here and close that out)

* I realize there are a few other places in the 2.x codebase not related to factories where things are returning null instead of false. I'll take a separate PR/issue to investigate and resolve these

jarednova · 2020-01-12T20:19:20Z

... and also to close the loop on CoAuthorsPlus. I think #2157 is the perfect place to either resolve or abandon attempting to make it compatible. Let's not let that block us here

2.x User factories DocBlocks

acobster · 2020-01-17T20:24:30Z

@jarednova yeah, I'm assuming the magic Timber getters will still return false when they don't find anything? In keeping with that I think it makes sense to return false here too.

Thanks for adding those tests, looking good 👍

jarednova · 2020-01-17T23:04:37Z

Awesome, feels great to get this merged!

jarednova · 2020-01-17T23:04:43Z

Awesome, feels great to get this merged!

Coby Tamayo and others added 12 commits January 7, 2020 16:41

#2089 implement public User API

a66d8a5

#2088 make User::__construct protected

82ddd5a

Like it says on the tin, this makes the User constructor protected. Also updates Post::author to use a Factory so class maps are supported when calling `Post::author()`.

#2093 guard against bad IDs in UserFactory

83282e3

#2094 fix most User API tests

9303137

#2093 fix copy/pasta

d678539

we're dealing with a user object, not a post :P

#2153 add a test helper for auto-removing filters

4a41e7f

Merge branch '2.x-factories' into 2.x-user-factories

e15adb9

Merge pull request #2158 from timber/2.x-temporary-hooks

fd7594f

#2153 add a test helper for auto-removing filters

#2094 fix Comment::author()

3f8620b

#2094 Fix CommentFactory ID logic

38d15ee

#2094 remove deprecated TimberUser Twig function

3ed2a06

#2094 update Twig User fn to use Timber::get_user

7d0a804

acobster requested review from gchtr and jarednova January 9, 2020 03:03

acobster requested a review from pascalknecht as a code owner January 9, 2020 03:03

jarednova added the 2.0 label Jan 9, 2020

#2093 test passing an empty array

43fd938

gchtr requested changes Jan 9, 2020

View reviewed changes

lib/Timber.php Show resolved Hide resolved

lib/Factory/UserFactory.php Outdated Show resolved Hide resolved

lib/Factory/UserFactory.php Show resolved Hide resolved

Coby Tamayo and others added 8 commits January 9, 2020 14:48

#2093 fix UserFactory::from_user_object() for 7.0

2f32a70

PHP below 7.2 does not support argument type declaration of `object`.

#2093 explicitly return in UserFactory::from()

42d5138

#2160 add UserFactory::from() doc comments

0acf074

more test group annotations

8fc1f1f

#2094 replace another User() isntantiation

775a93b

#2094 and another one

e9b0b5e

exclude coauthors from user tests

f344eee

#2157 drop support for Co-Authors Plus for now

f143bcd

gchtr mentioned this pull request Jan 12, 2020

2.x Add a Users Guide #2167

Merged

gchtr added 2 commits January 12, 2020 13:43

Improve DocBlocks

0fb6e07

Add DocBlock for User Class Map

d468c13

gchtr mentioned this pull request Jan 12, 2020

2.x User factories DocBlocks #2168

Merged

gchtr added 2 commits January 12, 2020 14:03

Add documentation for using Timber::get_users() with an array of user…

629e71b

… IDs

Add tests to check for invalid user IDs

6995c6f

gchtr approved these changes Jan 12, 2020

View reviewed changes

Assign time created to each post to ensure tests pass

1ed04c8

This comment was marked as outdated.

Sign in to view

jarednova added 3 commits January 12, 2020 14:36

Add test for invalid user comment class type; fix error message with …

42b706d

…such

Update non-result for get_user_by to be false instead of null; test f…

aeba8af

…or InvalidUserClass

Denote correct return type when no user found

d62ccde

jarednova approved these changes Jan 12, 2020

View reviewed changes

lib/Timber.php Show resolved Hide resolved

another null/false instance

b16a764

Merge branch '2.x-user-factories' into 2.x-user-factories-docs

0a959b7

jarednova mentioned this pull request Jan 12, 2020

2.x Add Timber::get_post_by() #2169

Merged

Merge pull request #2168 from timber/2.x-user-factories-docs

0d8b472

2.x User factories DocBlocks

acobster merged commit 9cf3b05 into 2.x-factories Jan 17, 2020

acobster deleted the 2.x-user-factories branch January 17, 2020 21:41

gchtr mentioned this pull request Mar 28, 2022

Use null instead of false as return type in factories #2561

Closed

Uh oh!

Conversation

acobster commented Jan 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Solution

Impact

Usage Changes

Considerations

Testing

Uh oh!

gchtr commented Jan 9, 2020

Uh oh!

acobster commented Jan 9, 2020

Uh oh!

gchtr commented Jan 9, 2020

Uh oh!

gchtr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

acobster commented Jan 11, 2020

Uh oh!

gchtr left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

jarednova commented Jan 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jarednova commented Jan 12, 2020

Uh oh!

acobster commented Jan 17, 2020

Uh oh!

jarednova commented Jan 17, 2020

Uh oh!

jarednova commented Jan 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

acobster commented Jan 9, 2020 •

edited

Loading

jarednova commented Jan 12, 2020 •

edited

Loading