Consider multisite for post abstraction.

[peterwilsoncc]

Describe the bug

Network connections make heavy use of the switch_to_blog() function. With regard to the post abstraction, this can be problematic if a helper method is called after switching blogs as the returned data can be a mix of content from the original site's post and the switched site's post.

For the methods within the post abstraction, it would be good to ensure the data always comes from the site used when the post abstraction was created.

Steps to Reproduce

1. Create a fresh multisite install of WordPress (example below uses subdirectory mode)
2. Network activate distributor using the develop branch.
3. On the main site, edit the hello world's post's title to Edited sample posts title
4. Create a subsite (it should have the site ID 2)
5. Run the following in the wp cli shell command.

   wp> $dt_post = new Distributor\DistributorPost( 1 );
   wp> $dt_post->get_permalink();
   string "http://ms-distributor.local/?p=1"
   wp> $dt_post->post->post_title
   string "Edited sample posts title" 
   
   wp> switch_to_blog( 2 );
   wp> $dt_post->get_permalink();
   http://ms-distributor.local/two/?p=1
   wp> $dt_post->post->post_title
   string "Edited sample posts title"   
   

Note that after switching to site two, the $dt_post variable is returning a mix of data from the primary and secondary site

Screenshots, screen recording, code snippet

No response

Environment information

No response

WordPress information

No response

Code of Conduct

• I agree to follow this project's Code of Conduct

[peterwilsoncc]

I created a mini-plugin to get some timing notes, times below are for the full number of runs, not averaged.

Multisite tests

Constructor only

checkout	60d805d (statics, no switching)
is_multisite	true
Timing test: 10000 runs	1.9749143123627

checkout	5c93a48 (switches to original site)
is_multisite	true
Timing test: 10000 runs	1.9749143123627

checkout	develop (bugs in methods when switched sites)
is_multisite	true
Timing test: 10000 runs	1.890456199646

::get_permalink() from switched blog

checkout	60d805d (statics, no switching)
is_multisite	true
Timing test: 10000 runs	0.017817497253418

checkout	5c93a48 (switches to original site)
is_multisite	true
Timing test: 10000 runs	2.1039011478424

checkout	develop (bugs in methods when switched sites)
is_multisite	true
Timing test: 10000 runs	N/A -- on a switched blog the data is incorrect

::get_permalink() from unswitched/original blog

checkout	60d805d (statics, no switching)
is_multisite	true
Timing test: 10000 runs	0.018847465515137

checkout	5c93a48 (switches to original site)
is_multisite	true
Timing test: 10000 runs	2.0892140865326

checkout	develop (bugs in methods when switched sites)
is_multisite	true
Timing test: 10000 runs	1.4913566112518

Single site tests

Constructor only

checkout	60d805d (statics, no switching)
is_multisite	false
Timing test: 10000 runs	1.9661166667938

checkout	5c93a48 (switches to original site)
is_multisite	false
Timing test: 10000 runs	1.9182817935944

checkout	develop (bugs in methods when switched sites)
is_multisite	false
Timing test: 10000 runs	1.9236187934875

::get_permalink() from unswitched/original blog

checkout	60d805d (statics, no switching)
is_multisite	false
Timing test: 10000 runs	0.017267942428589

checkout	5c93a48 (switches to original site)
is_multisite	false
Timing test: 10000 runs	1.5044174194336

checkout	develop (bugs in methods when switched sites)
is_multisite	false
Timing test: 10000 runs	1.3557698726654

[peterwilsoncc]

The one liner for this bug is: after running switch_to_blog() most of the helper methods return incorrect data.

After doing various tests I see the following solutions:

Use statics and pre-populate the data on multisite installs 60d805d

Pros:

• helper methods always return the original site/post's data
• after the first call, the methods run very fast

Cons:

• unfriendly to future devs: it's easy to add a method and forget the static/to prewarm it
• other plugins may add and/or remove filters during runtime that will be missed (similar to what distributor does for modified times)
• statics are impossible to test (although using properties in their place may solve this)

Check current site and switch if needed 5c93a48

Pros:

• helper methods will always get post data from the correct site
• the code is slightly simpler than above
• filters and actions can be modified during run time

Cons:

• Switching sites is slow
• unfriendly to future devs: it's easy to forget to add the swithing/restoring checks to future methods (potentially solvable with __call/__callStatic magic methods)
• even just checking for the need to switch is slow

Warn/fatal if using a helper method while the site is switched

I don't have any code for this but pretty much what it says on the pack, throw if someone tries to use a helper method while they've switched site.

Pros

• Possibly solvable with a __call magic method
• [untested but probably] less of a performance impact under normal use

Cons

• It's moving the problem to other developers/plugin users
• If a third party plugin switches sites and doesn't switch back, it looks like a bug in Dist rather than the other plugin

FWIW, I don't think this is a good idea. If the issue is solvable via defensive coding then we should do so. For one of the earlier suggestions, I might be able to be convinced to throw a notice for the performance impact.

[dkotter]

I've read through the above a few times but don't necessarily have a great opinion on the best approach.

I'm not opposed to the first approach where we're using statics but it's not a great pattern to follow and as mentioned, pretty hard to test. But seems like the best from a performance prospective. I'm wondering if this line although using properties in their place may solve this may be a good way to go, if that gives us all of the benefits without the downsides of statics?