Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Janitorial: Disparate cleanup tasks #40200
Janitorial: Disparate cleanup tasks #40200
Changes from all commits
c157034
01bb8ae
6e6eab8
d7458f0
3240990
4f83422
0674faf
3bdaac9
7842f7d
69aed20
565292b
1ed885c
460e14b
17b7094
5fd5720
f1f7025
e66c7aa
d143ebe
6355fae
a4aeab1
a9a6936
2a6feee
b0d8609
0dd6d17
a6fefc3
a792185
9fbfc42
6c32ce9
82bfe8e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a reasonable change, but doesn't seem to have anything to do with the PHP 7.2 change. Same for the similar stuff below.
I wonder if we even need the
class_exists( 'Jetpack' )
test? This seems to have been added in dd3b1d8 to try fixing static analysis.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed PR title.
As for whether we need it, I wasn't confident enough in the load order and knowledge of the environment to know if there's an edge case where the class might not exist. 😓
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a reasonable change, but doesn't seem to have anything to do with the PHP 7.2 change.
In this case, I don't see any indication as to why the
is_callable
was added in a4dea90, although Phan may again be the reason.The tricky part with adding a stub is that Simple has its own Jetpack class (fbhepr%2Skers%2Sjcpbz%2Sjc%2Qpbagrag%2Szh%2Qcyhtvaf%2Swrgcnpx%2Spynff.wrgcnpx.cuc%3Se%3Q37575q28-og) which doesn't entirely match Jetpack's, so whether we point to Jetpack's or make a stub from Simple's it'd be wrong for some environments. :sigh:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the PR title. Good to know about the Simple stub! At least in this case it still should be safe.