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

Plugin loading #855

Merged
merged 46 commits into from Nov 11, 2018

Conversation

@weirdan
Copy link
Contributor

commented Jul 2, 2018

Status

Ready for merge. Outstanding todo items can be added in subsequent PRs

Overview

Plugin installation/distribution system that piggy-backs on composer infrastructure. Follow-up to #849

Screenshot

image

Details

Available commands for psalm-plugin

  • psalm-plugin list - lists available and enabled plugins
  • psalm-plugin enable 'Plugin\Class\Name' - enables plugin (modifies psalm.xml)
  • psalm-plugin disable 'Plugin\Class\Name' - disables plugin (modifies psalm.xml)

Discovering plugins

https://packagist.org/packages/list.json?type=psalm-plugin

Plugin installation

composer install plugin-vendor/plugin-package-name

Plugin authoring

  • Skeleton plugin (usable with composer create-project ...): https://github.com/weirdan/psalm-plugin-skeleton
  • Plugins are identified by package type field, which should contain psalm-plugin string.
  • extra.psalm.pluginClass should refer to the name of the class implementing Psalm\PluginApi\PluginEntryPointInterface
  • Plugins are enabled by adding <pluginClass class="Qualified\Class\Name"/> to config file
  • Psalm\PluginApi namespace should be used to provide stable interfaces plugins can rely on

Sample plugin

Packagist: https://packagist.org/packages/weirdan/psalm-doctrine-collections
Git repo: https://github.com/weirdan/psalm-doctrine-collections

Todo

  • tests
  • better config file search (split and reuse)
  • cli argument to specify config file name
  • better output for psalm-plugin
  • phar compatibility
  • composer skeleton project for plugins
  • ability to refer to plugins by package name (cli only)
  • interface for plugin entry point
  • Plugin configuration (allows arbitrary content for pluginClass)

Further development

  • ~ migration path for legacy plugins~ Doesn't seem to be possible, because current plugins directly depend on Psalm internals (?)
  • better formatting for modified xml file
  • composer plugin to (optionally) enable plugin upon installation
  • documentation on plugin installation and authoring
  • interfaces for plugin dependencies
@weirdan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2018

@muglug here's the draft I promised. It's quite rough around the edges at the moment, but shows the general direction I had in mind.

@muglug

This comment has been minimized.

Copy link
Member

commented Jul 2, 2018

It's quite rough around the edges at the moment

I'll refrain from digging to deep, but I'm very on-board with command-line tools.

psalm-plugin enable 'Plugin\Class\Name'

why not use the composer name here?

@weirdan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2018

why not use the composer name here?

It's in todo ("ability to refer to plugins by package name (cli only)"). But if this is going to be used to manage non-composer plugins, there's also got to be a way to refer to plugins that are not packages. Since I don't want file names there, it leaves class names as the only choice.

@muglug

This comment has been minimized.

Copy link
Member

commented Jul 2, 2018

Ah sorry for missing that. I guess it's 100% fine to use either package name or classname there, as they can't overlap

This wouldn't work with a phar, right? Current plugins don't either, but I'm not massively bothered

@weirdan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2018

This wouldn't work with a phar, right?

I think it could be made to. Autoloading is functional in phar builds, so as long as interfaces used by plugin is not mangled by box rewriting it should just work.

Current plugins don't either, but I'm not massively bothered

Is it because their base class is prefixed?

@weirdan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2018

One point I'd like to decide before moving further is naming. How do we name this entire thing: "plugins", "extensions", "bundles", something else? "plugins" is currently ambiguous, as it refers both to old plugins as well to this new package based thing. My preference would be keep this named "plugins", add a LegacyPlugin adapter for old way of doing things and eventually deprecate it. But choosing a different name may also have its merits.

@muglug

This comment has been minimized.

Copy link
Member

commented Jul 2, 2018

Yeah, "plugins" is fine, I think. I don't know how many, if any, have implemented their own plugins outside of Vimeo, but this would be part of a V3 change if it broke them.

@muglug

This comment has been minimized.

Copy link
Member

commented Jul 4, 2018

Once your plugin is ready, it can go in https://github.com/psalm!

@weirdan weirdan force-pushed the weirdan:plugin-loading branch from 537e622 to 3f3fb77 Jul 4, 2018

docs/plugins.md Outdated Show resolved Hide resolved

@weirdan weirdan force-pushed the weirdan:plugin-loading branch from 73ebd1b to 98b3c97 Jul 9, 2018

@weirdan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2018

@muglug Is it ok to promote symfony/console from indirect dev-dependency to direct dependency? Would help with output and a bit with testability.

@weirdan weirdan force-pushed the weirdan:plugin-loading branch 2 times, most recently from 79305b5 to 140bf35 Jul 11, 2018

@coveralls

This comment has been minimized.

Copy link

commented Jul 12, 2018

Coverage Status

Coverage decreased (-0.02%) to 87.521% when pulling 903f5a4 on weirdan:plugin-loading into 6cf6d2c on vimeo:master.

@weirdan weirdan force-pushed the weirdan:plugin-loading branch 5 times, most recently from 2c6aace to 13c67cc Jul 13, 2018

@weirdan weirdan force-pushed the weirdan:plugin-loading branch 2 times, most recently from 806d9ea to 7c04717 Jul 23, 2018

@weirdan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2018

Skeleton plugin (usable with composer create-project ...): https://github.com/weirdan/psalm-plugin-skeleton

@weirdan weirdan force-pushed the weirdan:plugin-loading branch from 2b8f0ad to 82a469b Oct 29, 2018

@weirdan weirdan referenced this pull request Oct 31, 2018
4 of 4 tasks complete

@weirdan weirdan force-pushed the weirdan:plugin-loading branch from 82a469b to 66a4942 Nov 3, 2018

@weirdan weirdan changed the title [wip] Plugin loading Plugin loading Nov 3, 2018

@weirdan

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2018

@muglug I believe it's ready for merge.

@muglug

This comment has been minimized.

Copy link
Member

commented Nov 3, 2018

@weirdan great! I don’t have my computer with me for the next 24 hours, but I’ll review once I’m back

@muglug
Copy link
Member

left a comment

Got back a little later than intended, I've left some suggestions for documentation.

On a first glance this looks good - would you be able to migrate the plugins in the examples directory to the new format?

docs/plugins.md Outdated Show resolved Hide resolved
docs/plugins.md Outdated Show resolved Hide resolved
docs/plugins.md Outdated Show resolved Hide resolved
docs/plugins.md Outdated Show resolved Hide resolved
src/Psalm/Config.php Outdated Show resolved Hide resolved
weirdan added 21 commits Jul 23, 2018
cleaned up old cruft
- dropped todos I'm not going to pursues
- locked entry point to be a class implementing entry point interface
Added RegistrationInterface::registerHooksFromClass()
It mimics the way old plugins were registered in Psalm\Config, so
handler classes extending Psalm\Plugin should be fully compatible with
it.

Since Psalm\Plugin-style plugin registration was moved to
RegistrationSocket, LegacyPlugin now only load file-based plugins, so it
was renamed to FileBasedPluginAdapter.
Converted EchoChecker plugin to composer-based format
- Its subfolder is registered as a local composer package in the root
composer.json, so it's directly installable with
	```
	composer require psalm/echo-checker-plugin
	```
- Migration is trivial: drop the plugin into a separate folder, then add
simple composer.json and the entry point class.

@weirdan weirdan force-pushed the weirdan:plugin-loading branch from d182474 to 9f33edb Nov 6, 2018

@weirdan

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2018

@muglug all done

@muglug

This comment has been minimized.

Copy link
Member

commented Nov 7, 2018

Awesome, thanks. I'm going to make this part of a 3.0 release together with the Internal namespace changes. I'll change Psalm\Plugin to Psalm\LegacyPlugin or somesuch, with the API changes in the branch mentioned above. Should merge to master sometime this weekend. I'll create a 2.x branch to preserve that version so I can port select fixes where necessary.

weirdan added 2 commits Nov 8, 2018

@muglug muglug merged commit 052d4f6 into vimeo:master Nov 11, 2018

2 of 3 checks passed

coverage/coveralls Coverage decreased (-0.02%) to 87.521%
Details
Scrutinizer Analysis: 4 new issues, 48 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@weirdan weirdan deleted the weirdan:plugin-loading branch Nov 11, 2018

@muglug

This comment has been minimized.

Copy link
Member

commented Nov 14, 2018

Just created my first composer plugin: https://github.com/psalm/phpunit-psalm-plugin

It works! Thanks so much

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.