laminas / laminas-component-installer

Composer plugin for injecting modules and configuration providers into application configuration
https://docs.laminas.dev/laminas-component-installer/
BSD 3-Clause "New" or "Revised" License
27 stars 12 forks source link

Refactoring: type-safety along with some more simple code-paths #48

Closed boesing closed 2 years ago

boesing commented 2 years ago
Q A
Documentation no
Bugfix yes
BC Break yes
New Feature no

Description

This refactoring contains several changes:

Overall, the behavior is still the same. This just adds plenty of type-safety.

BC break only due to the whole final and native property-, argument- and return-typehints. I am not 100% sure if we have to create a new major release for this but if we want to do so, I am fine with that as well.

froschdesign commented 2 years ago

@boesing

BC break only due to the whole final and native property-, argument- and return-typehints. I am not 100% sure if we have to create a new major release for this but if we want to do so, I am fine with that as well.

Are there any side effects with other components or their installation? If not, then create a new major version.

boesing commented 2 years ago

Are there any side effects with other components or their installation? If not, then create a new major version.

Not 100% sure if I understand that question. Featureset here is equal to the current version. But if we consider strict semver for composer plugins (which should only be used as composer plugins and not as libraries) the same way we do on libraries, then we should create a new major. But when it comes to the end-result this plugin has to achieve (adding components to config files after asking the user during composer install, composer require, composer remove or composer update), we do not need to create a new major.

boesing commented 2 years ago

@weierophinney Please let me know what you would do differently. Always happy to get a different view. I just wanted to get rid of that reduce - maybe another rewrite without the whole collection stuff would be better but I wanted to keep functionality consistent without the need to rewrite every test.

weierophinney commented 2 years ago

I just wanted to get rid of that reduce

See, I prefer reduce to each + reference. :smile:

But probably the bigger refactor I'd do is to remove the collections stuff entirely; it was a solution in search of a problem at the time, and I think we could likely do this a lot more tightly with modern PHP than I did when I wrote this (more value objects, smaller public interface). But for now, this is a really good step towards better maintainability.

boesing commented 2 years ago

Okay. I guess releasing this as a new major will suffice. there are no dependants of this component and upgrading to the new major will be easy for those who are not using our code as library code. With the next major, everything is internal and starting with that, we can do whatever bc breaks we want to implement from an API perspective.

Cool for everyone?

I also added the other issues to the next major and I will work on this after my holidays which will start on 06-15. 👌🏻

boesing commented 2 years ago

Since I got no feedback, I guess we are good to go? I will back a release on sunday so there is still some time left for feedback tho.

Ocramius commented 2 years ago

New major works 👍