2.x Cleanup todo in code #2757

gchtr · 2023-05-30T13:00:45Z

Source code contains to be resolved issues "todos" #2626

Issue

Source code contains a lot of todos.

Solution

I worked through the todos in code as well as some skipped tests. I left the todos that are only about documentation.

I will add separate comments with the changes.

Impact

Make sure we didn’t forget anything important for 2.x

Usage Changes

None.

Considerations

None.

Testing

Yes.

gchtr · 2023-05-30T13:01:41Z

src/Comment.php

@@ -564,7 +564,6 @@ protected function avatar_host($email_hash)

    /**
     * @internal
-     * @todo  what if it's relative?


If this is an issue in the future, it will pop up as an issue.

gchtr · 2023-05-30T13:02:27Z

src/CoreEntity.php

@@ -145,7 +145,6 @@ protected function fetch_meta($field_name = '', $args = [], $apply_filters = tru
                $args
            );

-            // @todo Remove when deprecated filters will be gone


We can remove all apply_filters_deprecated() calls with the next major release.

gchtr · 2023-05-30T13:04:25Z

src/Factory/UserFactory.php

@@ -86,7 +86,6 @@ protected function from_user_object($obj): CoreInterface
        ));
    }

-    // @todo return a UserCollection instance?


Not at the moment. We discussed somewhere that user collections or collections for other types of content are not needed at the moment (#1689). We can add it if we really need it.

gchtr · 2023-05-30T13:05:00Z

src/Loader.php

@@ -587,7 +587,6 @@ public function clear_cache_twig()
        $twig = $this->get_twig();

        // Get the configured cache location.
-        // @todo What if this is a custom caching implementation?


This will pop up as an issue in case it becomes a problem.

gchtr · 2023-05-30T13:06:26Z

src/Loader.php

@@ -596,7 +595,6 @@ public function clear_cache_twig()
        }

        if (is_string($cache_location) && is_dir($cache_location)) {
-            // @todo What if not all files could be deleted?


Might be improved when we introduce better and streamlined error handling, see #2210.

gchtr · 2023-05-30T13:07:11Z

src/Post.php

@@ -47,7 +47,6 @@
 *     </div>
 * </article>
 * ```
- * @todo implement JsonSerializable?


Serialization can be added if needed. It’s documented in https://timber.github.io/docs/v2/guides/posts/#serialization.

gchtr · 2023-05-30T13:08:37Z

src/PostsIterator.php

@@ -35,7 +35,6 @@ public function current()
         */
        $wp_post = parent::current();
        // Lazily instantiate a Timber\Post instance exactly once.
-        // @todo maybe improve performance by caching the instantiated post.


Performance and caching is an area where we could improve in a lot of places. In case we want to improve post performance, we’ll find the right place to do it.

gchtr · 2023-05-30T13:09:17Z

src/Timber.php

@@ -318,7 +313,6 @@ public static function get_attachment($query = false, $options = [])
    {
        $post = static::get_post($query, $options);

-        // @todo make this determination at the Factory level.


We might improve this in the future. For the moment, it seems to work.

gchtr · 2023-05-30T13:09:30Z

src/Timber.php

@@ -457,9 +450,6 @@ public static function get_posts($query = false, $options = [])
            );
        }

-        /**
-         * @todo Are there any more default options to support?


We’ll figure them out if needed.

gchtr · 2023-05-30T13:10:01Z

src/Timber.php

@@ -863,7 +853,7 @@ public static function get_term_by(string $field, $value, string $taxonomy = '')
    public static function get_users(array $query = [], array $options = []): iterable
    {
        $factory = new UserFactory();
-        // TODO return a Collection type?


gchtr · 2023-05-30T13:10:28Z

tests/test-timber-deprecated.php

This file didn’t really do anything.

gchtr · 2023-05-30T13:11:11Z

tests/test-timber-post.php

@@ -178,25 +178,20 @@ public function testNextCategory()

    public function testNextCustomTax()
    {
-        $v = get_bloginfo('version');
-        if (version_compare($v, '3.8', '<')) {


We don’t need to support WordPress < 3.8.

gchtr · 2023-05-30T13:11:37Z

tests/test-timber-static.php

            'post_title' => 'Timmy',
        ]);
+
+        update_option('show_on_front', 'page');


This line was missing to get the test working.

gchtr · 2023-05-30T13:12:29Z

tests/test-timber.php

@@ -396,39 +390,6 @@ public function testGetPostWithClassMap()
        $this->assertInstanceOf(TimberAlert::class, Timber::get_post($alert_id));
    }

-
-
-    /* Terms API */


According to the coverage report, getting terms with different parameters is already covered in the factory tests.

gchtr · 2023-05-30T13:13:44Z

tests/test-timber.php

-    /**
-     * @expectedDeprecated Timber\Timber::query_post()
-     */
-    public function testQueryPost()


I guess this somehow tests whether Timber::query_post() would change the post global so that get_the_ID() returns the queried post. This logic is not supported anymore.

gchtr · 2023-05-30T13:15:43Z

tests/test-timber.php

-    {
-        // We don't actually test this directly in TestTimberPostGetter::testGettingWithFalse();
-        // that test directly instantiates a collection.
-        $this->markTestSkipped('@todo what should this be?');


With the newest version, when using false with Timber::get_posts(), the default query will be used. This test would test a deprecated functionality then. That’s why I removed it.

gchtr · 2023-05-30T13:16:17Z

tests/test-timber.php

@@ -901,48 +833,6 @@ public function testThePostHook()
        $this->assertSame(3, $the_post_count);
    }

-    public function testGetAttachment()
-    {
-        $this->markTestSkipped('@todo seems like a lot of what gets tested here is core WP file mgmt. Is that what we want?');


seems like a lot of what gets tested here is core WP file mgmt. Is that what we want?

I agree. Doesn’t seem to test anything worth testing that isn’t covered in other tests.

Levdbas

Nice work! That are a lot less todos!

szepeviktor · 2023-07-21T08:54:55Z

I feel a fresh smell.
Thank you!!

gchtr added 9 commits May 30, 2023 09:02

Remove stale todos

e650418

Clean up some todos in tests

a68cd59

Remove compatibility with older WordPress versions

c010fa2

Remove unneeded test

dd2f3a7

Remove class that does nothing

efab9da

Remove tests for deprecated functionality

7620017

Fix a test

98a7954

Remove check for non-supported WordPress version

9812196

Remove deprecated todo

7a71a26

gchtr added the 2.0 label May 30, 2023

gchtr commented May 30, 2023

View reviewed changes

tests/test-timber-deprecated.php

Copy link

Member Author

gchtr May 30, 2023

Choose a reason for hiding this comment

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

This file didn’t really do anything.

gchtr commented May 30, 2023

View reviewed changes

gchtr marked this pull request as ready for review May 30, 2023 13:16

gchtr requested review from jarednova and nlemoine as code owners May 30, 2023 13:16

gchtr added the Ready for Review Ready for a contrib to take a look at and review/merge label May 30, 2023

gchtr requested a review from Levdbas May 30, 2023 13:17

Levdbas approved these changes May 30, 2023

View reviewed changes

gchtr removed the Ready for Review Ready for a contrib to take a look at and review/merge label Jul 21, 2023

gchtr merged commit 49e4598 into 2.x Jul 21, 2023
76 of 77 checks passed

gchtr mentioned this pull request Jul 21, 2023

Source code contains to be resolved issues "todos" #2626

Closed

gchtr deleted the 2.x-todo branch July 21, 2023 08:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2.x Cleanup todo in code #2757

2.x Cleanup todo in code #2757

gchtr commented May 30, 2023

gchtr May 30, 2023

gchtr May 30, 2023

gchtr May 30, 2023

gchtr May 30, 2023

gchtr May 30, 2023

gchtr May 30, 2023

gchtr May 30, 2023

gchtr May 30, 2023

gchtr May 30, 2023

gchtr May 30, 2023

gchtr May 30, 2023

gchtr May 30, 2023

gchtr May 30, 2023

gchtr May 30, 2023

gchtr May 30, 2023

gchtr May 30, 2023

gchtr May 30, 2023 •

edited

Levdbas left a comment

szepeviktor commented Jul 21, 2023

2.x Cleanup todo in code #2757

2.x Cleanup todo in code #2757

Conversation

gchtr commented May 30, 2023

Issue

Solution

Impact

Usage Changes

Considerations

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gchtr May 30, 2023 • edited

Choose a reason for hiding this comment

Levdbas left a comment

Choose a reason for hiding this comment

szepeviktor commented Jul 21, 2023

gchtr May 30, 2023 •

edited