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

Move Collection to its own repository #1291

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

valzargaming
Copy link
Member

No description provided.

@valzargaming valzargaming requested a review from a team February 7, 2025 16:07
@Exanlv
Copy link
Collaborator

Exanlv commented Feb 9, 2025

Not gonna lie, I don't see how this is beneficial.

Just creates more overhead to maintain it separately?

@valzargaming
Copy link
Member Author

Several of the existing functions are outdated to be backwards compatible with old PHP versions. By moving it to a new repository we can introduce releases with their own PHP minimum versions and composer will automatically update to the end-user's most compatible up-to-date version without needing to force everybody to use PHP 8.4.

@valzargaming
Copy link
Member Author

valzargaming commented Feb 12, 2025

This actually makes maintaining the classes much easier as we can choose to drop backwards compatibility at any time in the future without needing to worry about rewriting functions at the same time, and it removes the extra effort from end-users as composer.json will handle choosing the appropriate version.

Both branches of the new repository will respect the CollectionInterface but have their own PHP version specific ways of doing things. The 8.4 branch contains the 3 additional array methods that were introduces with 8.4, and if there is enough of a demand for it we can add our own 8.0-specific implementation for it on the 8.0 branch, for example discord-php/Collection#3. It's a win-win and will save us time in the future.

@valzargaming valzargaming self-assigned this Feb 12, 2025
@Exanlv
Copy link
Collaborator

Exanlv commented Feb 12, 2025

I don't think its a good idea to reuse the namespace. Feels like youre just waiting for the autoloader to pull the class from the wrong place to me

@valzargaming
Copy link
Member Author

valzargaming commented Feb 12, 2025

I'm not sure I follow. Why would we want to change the namespace on an existing class within the library? The only difference is we've moved it to its own repository. I don't see how the autoloader would come into play here unless someone using the library explicitly tried to reuse our namespaces, which would be an issue even without this PR.

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

Successfully merging this pull request may close these issues.

2 participants