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.

Sign up for GitHub

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

Jump to bottom

Warn in log if user's WP version is too old for Jetpack #12341

Merged
merged 2 commits into from
May 23, 2019
Merged

Warn in log if user's WP version is too old for Jetpack #12341

merged 2 commits into from
May 23, 2019

Conversation

gravityrail
Copy link
Contributor

@gravityrail gravityrail commented May 11, 2019 •
edited
Loading

When I was testing another PR on VVV, the tests would fail with "class Jetpack not found" errors. It turns out that this was because I had an old (5.0-alpha) source tree in my VM, and there's an early return in jetpack.php if it detects an incompatible WordPress version.

My hope is that this change won't result in crazy verbose logging on sites with old versions of Jetpack, but... those sites are running an incompatible WordPress version anyway, so if it wasn't this weirdness it would be other weirdness. It also might be helpful in case they're missing the notices in their wp-admin.

Changes proposed in this Pull Request:

  • Warn in logs if Jetpack plugin can't load due to incompatible WordPress version.

@gravityrail gravityrail requested a review from a team as a code owner May 11, 2019 03:47
@gravityrail gravityrail self-assigned this May 11, 2019
@gravityrail gravityrail added the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label May 11, 2019
@jetpackbot
Copy link

jetpackbot commented May 11, 2019 •
edited
Loading

Warnings
⚠️ "Testing instructions" are missing for this PR. Please add some
⚠️ "Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against f1208a5

@kraftbj
Copy link
Contributor

kraftbj commented May 12, 2019

I think this would be pretty noisy running on every load, but this is a situation that generally only folks like us would find ourselves in.

WordPress itself won't let users update to versions of Jetpack that are indicated by us as needing a newer version of WordPress, so this would only be hit by folks updating Jetpack in weird ways.

Until recently we didn't return out at all until some of us developers fataled out when manually installing Jetpack on WP 4.9.

jeherve
jeherve requested changes May 15, 2019
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

Quick suggestion to avoid flooding too many error logs, and to ensure the message will be translatable.

jetpack.php Outdated Show resolved Hide resolved
@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels May 15, 2019
@jeherve jeherve added this to the 7.4 milestone May 15, 2019
@jeherve
Wrap error logging into WP_DEBUG check
f1208a5 
This should avoid flooding folks' error logs; we'll only log it if Debugging mode is on.

See #12341 (comment)
jeherve
jeherve approved these changes May 20, 2019
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This should be good to merge now that we are a bit more conservative, imo.

@kraftbj I'll let you merge if you are happy with this too.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels May 20, 2019
@jeherve jeherve merged commit be5560d into master May 23, 2019
@jeherve jeherve deleted the fix/warn-if-old-wp-version branch May 23, 2019 09:39
@matticbot matticbot removed the [Status] Ready to Merge Go ahead, you can push that green button! label May 23, 2019
jeherve added a commit that referenced this pull request May 23, 2019
@jeherve
Changelog: add #12341
2625d75
jeherve added a commit that referenced this pull request May 27, 2019
@kraftbj @jeherve
7.4 Changelog (#12304)
0fb4436 
* Kick off the changelog

* Add 7.3.1

* Update date and post link

* changelog: add #12219

* changelog: add #12170

* changelog: add #12184

* Changelog: add #12268

* Changelog: add #12081

* Changelog: add #12323

* Changelog: add #12204

* Changelog: add #12269

* Changelog: add #12332

* changelog: add #12339

* changelog: add #12209

* Changelog: add #12319

* Changelog: add #12357

* Changelog: add #12124

* Changelog: add #12373

* Changelog: add #12252

* Changelog: add #12383

* Changelog: add #12372

* changelog: add #12337

* Changelog: add #12290

* Changelog: add #12301

* Changelog: add #12061

* Testing list: add instructions for #12061

* Changelog: add #12393

* Update minimum supported version

See #12287

* Changelog: add #12406

* Testing list: add #12406

* Changelog: add #12277

* Changelog: add #12412

* Changelog: add #11318

* Changelog: add #12328

* Changelog: add #12425

* Changelog: add #12380

* Changelog: add #12428

* Changelog: add #12414

* Changelog: add #12395

* Changelog & Testing list: add #12416, #12417, #12418, and #12348

* changelog: add #12379

* Changelog: add #12341

* changelog: add #12444

* Changelog: add #12434

* Changelog: add #12454

* Changelog: add #12460

* Changelog: add #12463

* Changelog: add #12457

* Changelog / testing list: add #10333

* Changelog: add #12467


Co-authored-by: Jeremy Herve <jeremy@jeremy.hu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeherve jeherve jeherve approved these changes

Assignees

@gravityrail gravityrail

Labels
General [Pri] Low [Type] Janitorial
Projects
None yet
Milestone
7.4
Development

Successfully merging this pull request may close these issues.
5 participants
@gravityrail @jetpackbot @kraftbj @jeherve @matticbot