Skip to content

The 2.x Purge!#2354

Merged
jarednova merged 20 commits into2.xfrom
2323/purge_again
Nov 3, 2020
Merged

The 2.x Purge!#2354
jarednova merged 20 commits into2.xfrom
2323/purge_again

Conversation

@jarednova
Copy link
Copy Markdown
Member

@jarednova jarednova commented Oct 24, 2020 •
edited
Loading

Ticket: #2323

Issue

Remove the classes we identified in #2323

  • PostGetter

  • ... and tests

  • TermGetter

  • ... and tests

  • Any old $CommentClass props and the like

  • QueryIterator

  • ... and tests

  • Timber_WP_CLI_Command.php (or move to its own repo)

  • Post::determine_id() and possibly other methods

Solution

Delete the classes. Ensure that Factories and other 2.x mechanisms are fully functional without these.

Usage Changes

Everything in 2.x

Considerations

It's been well considered

Testing

Interested to see how this affects coverage score. Looking to bring that back up to come close to current 1.x levels

@coveralls
Copy link
Copy Markdown

coveralls commented Oct 25, 2020 •
edited
Loading

Coverage Status

Coverage increased (+4.9%) to 91.897% when pulling dab4d08 on 2323/purge_again into d9ec033 on 2.x.

@jarednova jarednova changed the title A nothing change to begin the PR and confirm current tests are in order The 2.x Purge! Oct 25, 2020
@acobster
Copy link
Copy Markdown
Collaborator

acobster commented Oct 29, 2020

QueryIterator ... and tests

@jarednova @gchtr I think we're ready to delete these. Of the remaining two tests in test-timber-query-iterator.php, one is already ported over into test-timber-post-collection.php. The last one is basically just testing Twig internals. I highly doubt there are any code paths through Timber being tested here that aren't already thoroughly tested elsewhere. Can you take a look and let me know if I'm missing anything?

Coby Tamayo added 5 commits October 29, 2020 14:20
#2323 vastly simplify Post instantiation
886b7fa 
This takes advantage of the fact that Post::init() only ever
receives a WP_Post object, thanks to Factories. As a result
we can eliminate many code paths that defend against not
having a fully fledged WP_Post.
#2323 simplify Post::init()
9bbffee
#2323 make Attachment more data-oriented
80a560e
#2323 go around init() completely
988d5f7
#2323 remove Post::init() and Attachment::init()
7816ceb
@gchtr gchtr added the 2.0 label Nov 2, 2020
@jarednova
Copy link
Copy Markdown
Member Author

jarednova commented Nov 3, 2020

@acobster looks like versions of both had actually been ported over to test-timber-post-collection.php. I went ahead and removed the file in my most recent commit. I think we're on to final review here!

@gchtr gchtr mentioned this pull request Nov 3, 2020
Merged
24 tasks
gchtr
gchtr approved these changes Nov 3, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@gchtr gchtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good to me. We only need to document this in the Upgrade Guide, but I’ve already noted that in the todos in #2073. The Upgrade Guide is further ahead there, so we should do it there.

@jarednova jarednova merged commit 729b3a9 into 2.x Nov 3, 2020
@jarednova jarednova deleted the 2323/purge_again branch November 3, 2020 21:34
@acobster
Copy link
Copy Markdown
Collaborator

acobster commented Nov 3, 2020

SO SATISFYING. 🎉 🔪

@jarednova
Copy link
Copy Markdown
Member Author

jarednova commented Nov 3, 2020

It's like losing fifty pounds, right? (or 1865 lines of code I guess)

gchtr added a commit that referenced this pull request Nov 10, 2020
@gchtr
#2354 Update Upgrade Guide
4072a41
@gchtr gchtr mentioned this pull request Nov 10, 2020
Merged
@acobster acobster mentioned this pull request Apr 13, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gchtr gchtr gchtr approved these changes

@acobster acobster Awaiting requested review from acobster

Assignees

No one assigned

Labels

2.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jarednova @coveralls @acobster @gchtr