Integrations API #2405

acobster · 2021-01-13T22:03:15Z

Ticket: #2402

Issue

New Integrations API!

Solution

For consistency with Factory and Image, renamed the namespace to Timber\Integration
Implemented the API per proposal What is the official way of initializing Timber? #2404
Preferring Gravatars in CoAuthors Plus meant you had to set CoAuthorsPlus::$prefer_gravatar = true which is awkward and non-idiomatic. Updated this to use a plain old filter instead.

Left to do

Document how to build a custom integration
Fix an issue with image URLs in WPML integration
Fix potential issues in test-timber-meta-deprecated.php
Not sure what Timber\Integrations\Command is for - I haven't dug into that code yet. But it's not one of the ones that was previously initialized in Integrations::__construct()...

Impact

Cleaner internal code, easier external integrations.

Usage Changes

This wasn't an external API until now, so usage changes will be covered by the introduction of docs for this new API.

Considerations

Please look carefully at the WPML integration and how it's tested. I'm less confident in that than I am in the others, because WPML has a bigger scope.

Testing

Reworked tests to use the new API. Tests were mostly unaffected.

This switches from using the public, static CoAuthorsPlusIntegration::$prefer_gravatar variable to using a basic custom filter for this part of the API. This is more consistent with WP World in general.

acobster · 2021-01-13T22:37:33Z

@gchtr the image resize test that's failing now (vendor/bin/phpunit --group integrations) is confusing to me. See my most recent commit. It seems to have to do with the "sideloaded" images that I believe you were working on recently - can you take a look?

gchtr · 2021-01-14T21:26:07Z

@gchtr the image resize test that's failing now (vendor/bin/phpunit --group integrations) is confusing to me. See my most recent commit. It seems to have to do with the "sideloaded" images that I believe you were working on recently - can you take a look?

That was a missing ICL_LANGUAGE_CODE constant for the WPML test. I hope that works now.

I moved all WPML-related tests to a new tests file that matches other integration tests. But I guess some things could still be moved around. We could probably use a setUp() function for the ICL_LANGUAGE_CODE constant, because that is a constant that is always set in a WPML environment.

acobster · 2021-01-15T01:32:50Z

We could probably use a setUp() function for the ICL_LANGUAGE_CODE constant, because that is a constant that is always set in a WPML environment.

Couple quick thoughts -

bootstrap.php is probably the better place for constants
If the constant is always set in a WPML environment, then the defined check will never fail, and we don't have a way to test the other code path (i.e. when ICL_LANGUAGE_CODE is not defined) anyway. Should we just remove that check?

gchtr · 2021-01-15T17:08:05Z

@acobster Yeah, probably bootstrap.php is a better place. Since we don’t actually install WPML when testing, setting that constant should work fine and then we wouldn’t need the defined() check.

* define ICL_LANGUAGE_CODE in test bootstrap * inline WPML directory setup (only used once)

acobster · 2021-01-15T20:10:37Z

@gchtr thanks for taking a look and consolidating those tests! Still not sure why the meta tests are failing. Also, please take a look at the new comments in test-timber-user.php - the tests pass now but if I add those same filters in a setUp method they fail, which indicates some implicit dependency to me. Haven't figure it out yet. I'd prefer to do it in setUp() because A.) it's cleaner, and B.) it uses the default Timber\User class across the board. Currently it's a CoAuthorsPlusUser instance, since the integration is global.

gchtr · 2021-01-17T17:02:20Z

🤔 It works for me if I use setUp(), including parent::setUp():

function setUp() {
    parent::setUp();

    $this->add_filter_temporarily('timber/user/classmap', function() {
        return User::class;
    });
}

acobster · 2021-01-18T05:32:25Z

Oh, duh! I was forgetting to call parent 😛

gchtr · 2021-01-18T07:48:49Z

🙈😅 Sometimes it’s that, sometimes it’s the order in which tests are called, sometimes it’s something else. No worries!

jarednova · 2021-01-20T02:28:49Z

Oh, duh! I was forgetting to call parent 😛

I just banged my head against a wall for 30 minutes on #2413 on this. @acobster is this something you see as a required 2.0 element? or something that belongs in the 2.1 camp?

acobster · 2021-01-21T21:02:44Z

I think 2.0 because we will need to refactor some of the ACF/meta stuff on top of this, and that stuff has at least one breaking change.

acobster · 2021-01-21T21:24:24Z

I will get around to fixing those User tests soon and then I think this will be solid enough to merge in. The meta tests that are failing will probably need to be revisited anyway as we change the API/integration with ACF etc.

gchtr

@gchtr work came knocking for me too, sorry for falling off the face of the Earth!

Same here :)

I quickly checked and found two things that look like stray debugging code (in separate comments).

And what’s the status of the following "Left to do" list?

Left to do

* Document how to build a custom integration

* Fix an image with image URLs in WPML integration

* Fix potential issues in `test-timber-meta-deprecated.php`

* Not sure what `Timber\Integrations\Command` is for - I haven't dug into that code yet. But it's not one of the ones that was previously initialized in `Integrations::__construct()`...

I think an Integration guide would be great. I’ll add it to my list of things to improve, but can’t guarantee when I can work on it 😅.

lib/ImageHelper.php

lib/Timber.php

This is a little cleaner/more modular than static methods here.

acobster · 2021-05-11T00:53:36Z

@gchtr thanks for looking! Those have been cleaned up.

Document how to build a custom integration

Guide added!

Fix an image with image URLs in WPML integration

I think this had to do with that leftover debug statement and is now fixed.

Fix potential issues in test-timber-meta-deprecated.php

Still running into issues where the tests expecting specific meta values are seeing null instead. Repro: vendor/bin/phpunit tests/test-timber-meta-deprecated.php

Not sure what Timber\Integrations\Command is for - I haven't dug into that code yet. But it's not one of the ones that was previously initialized in Integrations::__construct()...

I looked and that's just the WP-CLI integration. It's not really an "integration" in the same sense (WP-CLI is not a plugin), although we could treat it as such:

class WpCliIntegration implements IntegrationInterface {
  public function should_init() {
    return defined( 'WP_CLI' ) && WP_CLI;
  }

  public function init() {
    WP_CLI::add_command( 'timber', TimberCommand::class );
  }
}

I feel like it's fine to consider this out of scope here though. Command isn't used anywhere currently.

gchtr · 2021-07-12T19:33:25Z

Fix potential issues in test-timber-meta-deprecated.php

Still running into issues where the tests expecting specific meta values are seeing null instead. Repro: vendor/bin/phpunit tests/test-timber-meta-deprecated.php

I found the issue and fixed it in 5560141 (#2405). Classic "hook setup in a class" bug. If you want to make filters removable, you can’t use $this when adding the filter, because the filter will be added on the instance. If you want to remove the filter, you’ll have to get the exact instance.

That’s why the filters of the ACF integration were probably set up with static classes in the first place. I reverted it to what it was before, because that seemed to be the easiest change to fix this.

Another way to solve this would be to save the result of the initialized integrations in the Timber\Timber class and add a get_integrations() method that can be used to get the very instance we would have to use to remove the filters. But this is probably not needed for a couple of deprecated tests that will be removed with the next major version.

Document how to build a custom integration

Guide added!

Cool! I looked over it and saw that you added it in the /docs/v1 folder. I moved it to the v2 folder :).

And I added a note in the Upgrade Guide.

# Conflicts: # lib/Integration/AcfIntegration.php # tests/test-timber-integration-acf.php

jarednova · 2021-09-23T21:29:22Z

Hmmmm, it seems there's a failing test we'll need to resolve regarding how post meta data is stored/set ...

1) TestTimberMeta::testNonNullReturnInPreMetaFilterDisablesDatabaseFetch
Failed asserting that true matches expected false.

Error: /home/runner/work/timber/timber/tests/test-timber-meta.php:182

https://github.com/timber/timber/blob/feature/integrations-api/tests/test-timber-meta.php#L182

coveralls · 2021-09-28T18:19:09Z

Coverage decreased (-1.2%) to 89.963% when pulling 413bb1b on feature/integrations-api into c864b53 on 2.x.

gchtr · 2021-09-28T18:24:02Z

@jarednova I fixed the failing test in 17f1761 (#2405). Apart from the decreased coverage, I think this one is good to merge.

Coby Tamayo added 5 commits January 13, 2021 11:33

#2402 implement 2.0 Integrations API

90116cc

#2402 use a filter for controlling CoAuthors gravatars

d8a4814

This switches from using the public, static CoAuthorsPlusIntegration::$prefer_gravatar variable to using a basic custom filter for this part of the API. This is more consistent with WP World in general.

remove test of unsupported behavior

cc6d187

#2402 test WPML as part of integrations

0e9e7e2

#2402 Fix WPML integration (mostly)

6df64e3

acobster requested review from gchtr and jarednova as code owners January 13, 2021 22:03

acobster self-assigned this Jan 13, 2021

acobster marked this pull request as draft January 13, 2021 22:03

Coby Tamayo added 2 commits January 13, 2021 14:21

#2402 include image resize tests in integrations @group

883cf72

#2402 hacky bugfix attempt, debug code

afd7454

nlemoine mentioned this pull request Jan 14, 2021

2.x improve meta field handling #2397

Merged

Fix WPML tests

8e275e8

#2402 clean up WPML test

1534fd2

* define ICL_LANGUAGE_CODE in test bootstrap * inline WPML directory setup (only used once)

acobster marked this pull request as ready for review January 15, 2021 18:55

#2402 fix user tests

7488df4

acobster added the 2.0 label Jan 20, 2021

Coby Tamayo added 2 commits April 9, 2021 13:37

Merge branch '2.x' into feature/integrations-api

8b20363

#2402 fix user test setUp() by calling parent

731268c

gchtr reviewed May 8, 2021

View reviewed changes

lib/ImageHelper.php Outdated Show resolved Hide resolved

lib/Timber.php Outdated Show resolved Hide resolved

Coby Tamayo added 3 commits May 10, 2021 16:21

Remove debug dump/echo statements

8a1b4df

Define AcfIntegration methods as instance methods

fc58699

This is a little cleaner/more modular than static methods here.

#2402 add an Integrations API guide

8f5b574

Coby Tamayo and others added 8 commits May 10, 2021 18:03

annotate php code block

fcd0553

Fix whitespace in example

5c511c1

adjust IntegrationInterface indentation for consistency

191f978

fix a stray single quote - shoulda been a backtick

ecb09c7

Fix test issues in TestTimberMetaDeprecated

5560141

Move Custom Integrations guide to v2 folder

e79604d

Update Upgrade Guide

ad38946

Add DocBlock for timber/integrations filter

a52e026

Merge branch '2.x' into feature/integrations-api

d093ae7

gchtr unassigned acobster Sep 21, 2021

Merge branch '2.x' into feature/integrations-api

a249794

# Conflicts: # lib/Integration/AcfIntegration.php # tests/test-timber-integration-acf.php

gchtr added 4 commits September 28, 2021 20:07

Fix failing test

17f1761

Fix missing User class error in tests

61a7506

Remove unused use statements

53511ba

Merge branch '2.x' into feature/integrations-api

413bb1b

gchtr approved these changes Sep 28, 2021

View reviewed changes

jarednova approved these changes Sep 28, 2021

View reviewed changes

jarednova merged commit 1714b01 into 2.x Sep 28, 2021

jarednova deleted the feature/integrations-api branch September 28, 2021 19:53

jarednova added a commit that referenced this pull request Sep 28, 2021

Remove incorrectly restored test removed in #2405

b2df460

gchtr mentioned this pull request Oct 1, 2021

2.0 Integrations API #2402

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrations API #2405

Integrations API #2405

acobster commented Jan 13, 2021 •

edited by gchtr

acobster commented Jan 13, 2021

gchtr commented Jan 14, 2021

acobster commented Jan 15, 2021

gchtr commented Jan 15, 2021

acobster commented Jan 15, 2021

gchtr commented Jan 17, 2021

acobster commented Jan 18, 2021

gchtr commented Jan 18, 2021

jarednova commented Jan 20, 2021

acobster commented Jan 21, 2021

acobster commented Jan 21, 2021

gchtr left a comment

acobster commented May 11, 2021

gchtr commented Jul 12, 2021

jarednova commented Sep 23, 2021

coveralls commented Sep 28, 2021

gchtr commented Sep 28, 2021

Integrations API #2405

Integrations API #2405

Conversation

acobster commented Jan 13, 2021 • edited by gchtr

Issue

Solution

Left to do

Impact

Usage Changes

Considerations

Testing

acobster commented Jan 13, 2021

gchtr commented Jan 14, 2021

acobster commented Jan 15, 2021

gchtr commented Jan 15, 2021

acobster commented Jan 15, 2021

gchtr commented Jan 17, 2021

acobster commented Jan 18, 2021

gchtr commented Jan 18, 2021

jarednova commented Jan 20, 2021

acobster commented Jan 21, 2021

acobster commented Jan 21, 2021

gchtr left a comment

Choose a reason for hiding this comment

Left to do

acobster commented May 11, 2021

gchtr commented Jul 12, 2021

jarednova commented Sep 23, 2021

coveralls commented Sep 28, 2021

gchtr commented Sep 28, 2021

acobster commented Jan 13, 2021 •

edited by gchtr