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

Feature Hints: disable when plugins cannot be installed on site. #14108

Merged
merged 2 commits into from
Nov 25, 2019
Merged

Feature Hints: disable when plugins cannot be installed on site. #14108

merged 2 commits into from
Nov 25, 2019

Conversation

jeherve
Copy link
Member

@jeherve jeherve commented Nov 22, 2019 •
edited
Loading

Changes proposed in this Pull Request:

  • When one cannot install plugins, we do not have to do anything with feature hints.

Internal reference: p1571740982220600-slack-jetpack-crew

Testing instructions:

I am not quite sure to be honest. @mjangda Any idea here?

  • Add define( 'DISALLOW_FILE_MODS', true ); to your site's wp-config.php file.
  • From that point on, you cannot access wp-admin/plugin-install.php on your site so Feature hints should not be triggered.
  • Remove the constant, head over to Plugins > Add New, and type "cdn" in the search field.
  • If the Asset CDN feature is not already enabled on your site, you should see a prompt to enable it.

Proposed changelog entry for your changes:

  • Feature Hints: disable when plugins cannot be installed on site.

@jeherve
Feature Hints: disable when plugins cannot be installed on site.
d56a04c 
When one cannot install plugins, we do not have to do anything with feature hints.
@jeherve jeherve added [Type] Bug When a feature is broken and / or not performing as intended [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Pri] Low Plugin Search aka Feature Hints labels Nov 22, 2019
@jeherve jeherve requested review from mjangda and a team November 22, 2019 20:14
@jeherve jeherve self-assigned this Nov 22, 2019
@jetpackbot
Copy link

jetpackbot commented Nov 22, 2019 •
edited
Loading

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: December 3, 2019.
Scheduled code freeze: November 26, 2019

Generated by 🚫 dangerJS against 983d9e8

@jeherve jeherve mentioned this pull request Nov 22, 2019
Closed
@mjangda
Copy link
Member

mjangda commented Nov 22, 2019

I am not quite sure to be honest.

Yeah, kind of hard to test, but one way to verify would be to check Jetpack_Plugin_Search::$instance === null when the constant is enabled (via wp shell, for example).

@mjangda
Copy link
Member

mjangda commented Nov 22, 2019

You also obviously want to test the reverse: that plugin search hints are still active when the constant is not set or false :)

@mjangda mjangda mentioned this pull request Nov 22, 2019
Closed
4 tasks
@jeherve
Copy link
Member Author

jeherve commented Nov 22, 2019

You also obviously want to test the reverse

You're right, I should probably mention that :)

@jeherve jeherve added this to the 8.0 milestone Nov 22, 2019
dereksmart
dereksmart approved these changes Nov 25, 2019
View reviewed changes
Copy link
Member

@dereksmart dereksmart left a comment

Choose a reason for hiding this comment

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

Works well. Tested that plugins hints still work when the constant is missing as well as declared false

@dereksmart dereksmart added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 25, 2019
@jeherve jeherve merged commit 5f3a9b5 into master Nov 25, 2019
@jeherve jeherve deleted the disable/psh-no-plugininstall branch November 25, 2019 16:19
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Nov 25, 2019
jeherve added a commit that referenced this pull request Nov 25, 2019
@jeherve
Changelog: add #14108
c760986
jeherve added a commit that referenced this pull request Nov 25, 2019
@jeherve
8.0 Release: running changelog (#13964)
807ab89 
* 8.0 Release: running changelog

* Changelog: add #13921

* Changelog: add #13980

* Changelog: add #13905

* Changelog: add #13971

* Changelog: add #13984

* Changelog: add #14009

* Changelog: add #13620

* Remove things that will ship in 7.9.1

* Changelog: add 7.9.1 release (#14044)

* Changelog: add base for 7.9.1 release

* Update release date and post link

* Changelog: add #14066

* Update changelog for 7.9.1

* Changelog: add #13405

* Changelog: add #13841

* Changelog: add #13924

* Changelog: add #13986

* Changelog: add #14010, #14028, #14053, #14055.

* Changelog: add #14054

* Changelog: add #14031

* Changelog: add #14039

* Changelog: add #14050

* Changelog: add #14070

* Changelog: add #14082

* Changelog: add #14084

* Changelog: add #14111

* Changelog: add #13961

* Changelog: add #14047

* Changelog: add #14091

* Changelog: add #14108

* Changelog: add #14121
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjangda mjangda mjangda left review comments

@dereksmart dereksmart dereksmart approved these changes

Assignees

@jeherve jeherve

Labels
Plugin Search aka Feature Hints [Pri] Low [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Milestone
8.0
Development

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