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

Improve Decoupling and requirements between uri packages #137

Open
nyamsprod opened this issue Jun 2, 2024 · 5 comments
Open

Improve Decoupling and requirements between uri packages #137

nyamsprod opened this issue Jun 2, 2024 · 5 comments
Assignees
Labels
enhancement New feature or request

Comments

@nyamsprod
Copy link
Member

nyamsprod commented Jun 2, 2024

Feature Request

Q A
Package no
New Feature no
BC Break yes/no

Introduction

League uri packages are made up of three (3) packages:

  • uri-interfaces: contains contracts and utility classes to perform primitive process around URI
  • uri: contains concrete classes to handle complete URI and URI templates and depends on uri-interfaces
  • uri-components: contains concrete classes to handle URI components and depends on uri.

Having the contracts being defined on uri-interfaces makes the dependency a bit too complex to decouple. For instance the uri package does not depend nor does it requires uri-components interfaces. Having those interface being loaded when requiring the uri package make little sense.

Using the same argument the uri package does not require the PSR-7 interfaces. The package provides a PSR-7 UriInterface implementation. But that implementation is optional and is not a requirement for the package to work. So putting the PSR-7 and PSR-17 requirements in the suggest section of the composer file seems reasonable. If compatibility with PSR interfaces is a requirement explicitly requiring the extra package is a better option. It makes the dependency clearer and cleaner from a developer POV. The goal of this feature is to

  • reduce the dependency tree
  • makes it more clearer the goal of each separate packages.

We have two proposals:

Proposal 1:

  • We remove all interfaces from the uri-interfaces packages which will only contains utility classes.
  • We move all the interfaces related to URI processing in the uri package.
  • We make the PSR-7/PSR-17 interfaces optional for the uri package.
  • We move all the interfaces related to URI component processing in the uri-component package.
  • We make the PSR-7/PSR-17 interfaces optional for the uri-component package.

The consequences:

  • Introduce a BC to the uri-interfaces which will be bumped to v8.
  • uri dependencies stays as is
  • uri-components dependencies stays as is
  • uri and uri-components will only be bump to a minor version v7.5.

Issues/Open questions:

  • The current subsplit tool used in uri-src is unable to handle 2 major versions release at the same time.
  • The UriAccess interface uses the PSR-7 UriInterface in a union type for a return type. Is that affected by having PSR-7 classes optional inside the uri-package ?

Proposal 2:

  • We create a new package uri-helpers which will only contains utility classes.
  • We move all the interfaces related to URI processing in the uri package.
  • We make the PSR-7/PSR-17 interfaces optional for the uri package.
  • We move all the interfaces related to URI component processing in the uri-component package.
  • We make the PSR-7/PSR-17 interfaces optional for the uri-component package.

The consequences:

  • uri will now depend only on uri-helpers
  • uri-components dependencies stays as is
  • the uri-interfaces package is deprecated (ie does not receive any updated and will still be requested and installed when using older version of uri version.
  • all packages are bumps to 7.5 (including uri-helpers)

Issues/Open questions:

  • Should we remove from the monorepo uri-interfaces to be sure it has been deprecated and no longer received automatic update. We still need to keep it in line in case of security issue with the new uri-helpers package.
  • Since we changed dependencies for uri in between version can this be considered as BC break for the uri package or not ?
    According to this issue it does not: Version after dependency changes? semver/semver#148
  • The UriAccess interface uses the PSR-7 UriInterface in a union type for a return type. Is that affected by having PSR-7 classes optional inside the uri-package ?
  • uri-helpers will conflict with uri-interfaces normally the conflict should be minimal on update uri-interfaces package should be replaced by uri-helpers. A conflict can only arise if someone did require only the uri-interfaces and then later on requires a version higher than 7.4 of any other package. But this can be solve by either removing uri-interfaces or by installing uri-components.

Future scope

Once the Interfaces have been moved to their appropriate package we should question their utilities
and if they should be removed/replace/split or if we should add more. Those changes should be examined in a context of a major BC break and for inclusion in the next major release for the library.

@nyamsprod nyamsprod added the enhancement New feature or request label Jun 2, 2024
@nyamsprod nyamsprod self-assigned this Jun 2, 2024
@nyamsprod
Copy link
Member Author

nyamsprod commented Jun 3, 2024

@wouterj @kelunik @shadowhand For context the packages were mentioned in regards to a PR in Symfony to include a new URI component. One of the blocker was the PSR-7 dependency of all the packages. IMHO this dependency is important but not crucial for the package to work. Hence I put together the following proposal. Your thoughts, remarks, suggestions are welcomed.

These are ideas no definitive solutions. The goal is to improve the situation without adding more headache to users of the packages.

Of note: regardless of Symfony decision I still believe this is still something we need to address whether the solution lands now or when developing the next major release whichever comes first.

@shadowhand
Copy link
Member

This is a pretty amazing opportunity to contribute years of work and knowledge to a foundational part of the PHP ecosystem, congrats!

My perfect solution for this situation would be:

  • Make a separate league/http-message-uri package that depends on league/uri and psr/http-message and provides translation between PSR-7 interfaces and league/uri interfaces.
  • Donate league/uri to Symfony (as symfony/uri) and deprecate this package. If Symfony creates a separate URI implementation, this package will almost certainly see usage dry up. If you want to continue to share your knowledge and skill in this domain, perhaps Symfony would allow you to continue maintain the package?

For what it is worth, I think the current repository split is confusing and makes it harder to contribute to this package. I worry about trying to do too many things in one place, as opposed to a core package with dependents.

@wouterj
Copy link

wouterj commented Jun 3, 2024

Donate league/uri to Symfony (as symfony/uri) and deprecate this package. If Symfony creates a separate URI implementation, this package will almost certainly see usage dry up. If you want to continue to share your knowledge and skill in this domain, perhaps Symfony would allow you to continue maintain the package?

This is purely my personal opinion, but keeping league/uri in the PHP league organization and Symfony using it instead of creating symfony/uri is a valid option as well (which is what we're currently discussing in Symfony).
Does that change your perfection solution? (i.e. does that still make donating the package to Symfony the ideal scenario?)

@shadowhand
Copy link
Member

@wouterj yes, I still think donating the whole package is the ideal scenario.

@nyamsprod
Copy link
Member Author

@shadowhand before donating something one has to wonder what is the endgoal for Symfony. At the moment the developers from Symfony are not clear in their own intention and I still reserved the right to not donate the package but I think/hope I understand your argumentation for such action.

Having said that I am still open for a debate on what is best for the community at large where that solution is depend fully on us knowing if we can or not collaborate and meet half way in any given solution.

But it does not preclude the fact that some architectural changes are still needed in the package and those can be made gradually via a minor releases or more aggressively via a major release and that is the main reason for the ticket.

Making the package "more attractive" for anyone to work on it is a side effect of those decisions not the otherway around at least that is how I see it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants