Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent unneeded blog switching in multisite env. #2781

Merged
merged 7 commits into from Feb 27, 2024

Conversation

Levdbas
Copy link
Member

@Levdbas Levdbas commented Jul 25, 2023

To get some further experience with PRs I wanted to tackle an older issue.

Related:

Issue

switch_to_blog() was used when not needed in multisite environments, resulting in unneeded globals being set.

Solution

Only use switch_to_blog() when the ID is different from the current id blog id. While at it, I changed some internal variables to reflect the new internal working and updated the docblock as well. I also added a fallback when get_blog_details() returns null instead of a site object.

Impact

Less globals set in the form of $GLOBALS['_wp_switched_stack'] and $GLOBALS['switched']

Testing

Current tests are covering it.

@gchtr gchtr added the 2.0 label Jul 25, 2023
@Levdbas Levdbas changed the title 1180 timbersite construct breaks image srcset Prevent unneeded blog switching in multisite env. Jul 25, 2023
@coveralls
Copy link

coveralls commented Jul 25, 2023

Coverage Status

coverage: 88.422% (-0.3%) from 88.676% when pulling 3e45f8a on 1180-timbersite__construct-breaks-image-srcset into 129b9c6 on 2.x.

Copy link
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.

Good catch!

I only found some issues with strict types, otherwise this looks good to me.

src/Site.php Outdated Show resolved Hide resolved
src/Site.php Outdated Show resolved Hide resolved
src/Site.php Outdated Show resolved Hide resolved
@gchtr gchtr added 2.x Future and removed 2.0 labels Jul 31, 2023
@gchtr
Copy link
Member

gchtr commented Jul 31, 2023

@Levdbas I’ve marked this as 2.x future, because it’s not essential to include this in the Release Candidate for Timber 2.0 ;).

@Levdbas
Copy link
Member Author

Levdbas commented Aug 1, 2023

@Levdbas I’ve marked this as 2.x future, because it’s not essential to include this in the Release Candidate for Timber 2.0 ;).

No problem! I think I managed to fix the PR though. Prior to this PR Timber was always switching to a blog, no matter what. After my initial changes Timber was still always changing because of the strict checking and without transforming the $blog_identifier in an INT first, still resulting in always switching in these tests and thus passing the tests.

After applying your suggestions, it finally passed the strict testing and resulting in not switching for the first time but resulting in testing errors.

What I did to make the tests work again:

Add restore_current_blog(); after each switch blog function, just like it is recommended by WordPress. Adding these on rule 149, 151, 153 was needed in the end to fix the test.

I think this is because self::createSubDomainSite switches to a particular blog as well. Since we ended up preventing unnecessary switching, the get_post() call was running on the wrong website at

$current_site_all_posts = get_posts();
resulting in a wrong test result.

So by setting the restore_current_blog(); we made sure each time we make sure that the starting blog is correct for making the query.

Let me know what your thoughts.

@Levdbas Levdbas added 2.0 and removed 2.x Future labels Dec 13, 2023
@Levdbas Levdbas linked an issue Dec 13, 2023 that may be closed by this pull request
@Levdbas Levdbas added this to the 2.0.1 milestone Dec 14, 2023
@Levdbas Levdbas modified the milestones: 2.0.1, 2.1.0 Feb 1, 2024
@Levdbas Levdbas requested a review from gchtr February 24, 2024 08:52
@Levdbas Levdbas merged commit d81f995 into 2.x Feb 27, 2024
51 of 52 checks passed
@gchtr gchtr deleted the 1180-timbersite__construct-breaks-image-srcset branch February 27, 2024 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

\Timber\Site\__construct breaks image srcset
3 participants