Skip to content

❗Allow plugins to modify Config::$fileExtensions early#6789

Merged
orklah merged 2 commits intovimeo:masterfrom
ohader:issue-6788
Jan 30, 2022
Merged

❗Allow plugins to modify Config::$fileExtensions early#6789
orklah merged 2 commits intovimeo:masterfrom
ohader:issue-6788

Conversation

@ohader
Copy link
Copy Markdown
Contributor

@ohader ohader commented Nov 1, 2021 •
edited
Loading

ProjectAnalyzer consumed Config::$fileExtensions early in its
constructor - without having processed plugins' modifications,
registering their custom scanners or analyzer implementations.

This change

  • adds new specific interface \Psalm\Plugin\FileExtensionsInterface
    to be used by plugin implementations
  • extracts file extension handling from \Psalm\PluginRegistrationSocket
    and interface \Psalm\Plugin\RegistrationInterface to a new dedicated
    \Psalm\PluginFileExtensionsSocket and new interface
    \Psalm\Plugin\FileExtensionsInterface
    !!! this is a breaking change in PluginRegistrationSocket !!!
  • adds runtime in-memory cache for Config::$plugins
  • calls new method Config::processPluginFileExtensions(), providing
    modifications to file extension only early in ProjectAnalyzer
  • adjusts documentation

Adjusted test scenario https://github.com/ohader/psalm-6788/tree/adjusted

Fixes: #6788

Deprecation in 4.x branch, see #7455

@ohader ohader force-pushed the issue-6788 branch 2 times, most recently from c1eb612 to 0dfb003 Compare November 1, 2021 20:27
@ohader ohader changed the title Initialize plugins before consuming Psalm\Config settings ❗Allow plugins to modify Config::$fileExtensions early Nov 1, 2021
@ohader ohader force-pushed the issue-6788 branch 3 times, most recently from 1a4aac6 to bddf808 Compare November 1, 2021 21:10
@ohader
Copy link
Copy Markdown
Contributor Author

ohader commented Nov 1, 2021

Okay, CI-wise

@ohader ohader marked this pull request as ready for review November 1, 2021 21:20
@orklah orklah added the release:feature The PR will be included in 'Features' section of the release notes label Nov 1, 2021
ohader added a commit to ohader/psalm-6788 that referenced this pull request Nov 1, 2021
@ohader
Adjust plugin implementation for vimeo/psalm#6789
d0ed0ed
@orklah
Copy link
Copy Markdown
Collaborator

orklah commented Nov 1, 2021

failure is indeed unrelated.

If this is a breaking change, I'm afraid this will have to wait for Psalm 5

@ohader
Copy link
Copy Markdown
Contributor Author

ohader commented Nov 1, 2021

failure is indeed unrelated.

Thanks for picking this up so fast!

If this is a breaking change, I'm afraid this will have to wait for Psalm 5

In case somebody called the functionality of addFileTypeScanner or addFileTypeAnalyzer already in their custom plugins' __invoke() method , it will cause a PHP fatal error since the methods have been moved.

As a non-breaking (but also non-functional) fall-back I could think of keeping empty stub methods still in PluginRegistrationSocket, in particular keeping

@orklah
Copy link
Copy Markdown
Collaborator

orklah commented Nov 2, 2021

Yeah, this might be better if you want to ship this soon. Please add a deprecated tag so we know to remove it on Psalm 5

@orklah orklah marked this pull request as draft December 10, 2021 21:35
@orklah
Copy link
Copy Markdown
Collaborator

orklah commented Jan 20, 2022

Hi @ohader, just a friendly notice: Psalm 5 will be coming soonish. If you want to have a chance of getting this merged before Psalm 6, deprecation should be commited on 4.x branch, this already targets master so it should be okay.

@ohader
Copy link
Copy Markdown
Contributor Author

ohader commented Jan 20, 2022

@orklah thanks for the notification. Can you please give me a hint how deprecations are handled (error trigger, documentation, etc) in Psalm? Thx in advance!

@orklah
Copy link
Copy Markdown
Collaborator

orklah commented Jan 20, 2022

flag any removed method/property/class as @deprecated with a description. If it's not a full removal, just add some phpdoc to explain the change.

In any case, UPGRADE.md need to be updated on master branch to explain each BC break

@ohader
Copy link
Copy Markdown
Contributor Author

ohader commented Jan 21, 2022

@orklah

@ohader
Copy link
Copy Markdown
Contributor Author

ohader commented Jan 21, 2022

Not sure what the idea behind Use of numeric literal separator is required was, but... 1_635_800_582 seems to work... it's actually just a unix-timestamp to have unique identifiers for exceptions...

@ohader ohader marked this pull request as ready for review January 21, 2022 15:26
@ohader
Copy link
Copy Markdown
Contributor Author

ohader commented Jan 21, 2022

@orklah Concerning release of Psalm v5: Is there a schedule/date yet? I might have some smaller changes/fixes and would focus on them during the weekend...

@orklah
Copy link
Copy Markdown
Collaborator

orklah commented Jan 21, 2022

@ohader we were thinking about releasing an alpha in the next few days but it doesn't mean feature freeze yet. We still have a few weeks before release date, however we don't have a precise schedule

@orklah
Copy link
Copy Markdown
Collaborator

orklah commented Jan 25, 2022

Can you rebase and ping me? I'll merge this

ohader added 2 commits January 30, 2022 13:06
@ohader
!!! Allow plugins to modify Config::$fileExtensions early
62a0ece 
ProjectAnalyzer consumed Config::$fileExtensions early in its
constructor - without having processed plugins' modifications,
registering their custom scanners or analyzer implementations.

This change
* adds new specific interface \Psalm\Plugin\FileExtensionsInterface
  to be used by plugin implementations
* extracts file extension handling from \Psalm\PluginRegistrationSocket
  and interface \Psalm\Plugin\RegistrationInterface to a new dedicated
  \Psalm\PluginFileExtensionsSocket and new interface
  \Psalm\Plugin\FileExtensionsInterface
  !!! this is a breaking change in PluginRegistrationSocket !!!
* adds runtime in-memory cache for Config::$plugins
* calls new method Config::processPluginFileExtensions(), providing
  modifications to file extension only early in ProjectAnalyzer
* adjusts documentation
@ohader
Apply code styles
3fedb5c
@ohader
Copy link
Copy Markdown
Contributor Author

ohader commented Jan 30, 2022

@orklah Rebased. My brain was busy with totally unrelated tasks during weekdays, sorry for the delay.

@orklah orklah merged commit 64d06c6 into vimeo:master Jan 30, 2022
@orklah
Copy link
Copy Markdown
Collaborator

orklah commented Jan 30, 2022

Thanks!

@ohader ohader deleted the issue-6788 branch January 30, 2022 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

PR: Need review release:feature The PR will be included in 'Features' section of the release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Plugins cannot register their own file type scanners/analyzers

2 participants

@ohader @orklah