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

Fix Marketplace falsely thinks a plugin is installed when it is not #17278

Merged
merged 2 commits into from Mar 7, 2021

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Feb 26, 2021

Description:

Notices this earlier on demo2. We had removed various plugins from the disk but the config still had the plugins marked as installed. Then I wanted to reinstall the plugins but the marketplace wouldn't let me install it because it was thinking the plugin is already installed. That's because it was only looking at PluginsInstalled[] entries in the config but not if the plugin actually still existed in the file system.

This change now makes plugins installable again in such a case. I noticed few other usages of isPluginInstalled() but not always would we actually want to check if the plugin is in the filesystem. Either from a performance point of view (eg in the component update checker) or because the logic doesn't need/want it.

For now just wanted to fix this specific case. Seems there are no tests so far for the plugin manager but can add one if needed.

Review

  • Functional review done
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@tsteur tsteur added the Needs Review PRs that need a code review label Feb 26, 2021
@tsteur tsteur added this to the 4.3.0 milestone Feb 26, 2021
Copy link
Contributor

@flamisz flamisz left a comment

Choose a reason for hiding this comment

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

👍

@diosmosis
Copy link
Member

@tsteur is this something we'd want covered by a test? (I can add one if needed)

@tsteur
Copy link
Member Author

tsteur commented Mar 2, 2021

@diosmosis added test for the plugins manager 👍 I reckon it might not easy to add a test for the marketplace where it thinks the plugin is installed but actually missing from the files since the marketplace would also need to show that plugin (meaning it would need to exist on the marketplace). I'm thinking a test wouldn't be needed

@diosmosis diosmosis merged commit 428a4b9 into 4.x-dev Mar 7, 2021
@diosmosis diosmosis deleted the marketplaceinstallfix branch March 7, 2021 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants