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

Removed nullable type hint from $options parameter in routes. #5509

Closed
wants to merge 1 commit into from

Conversation

iRedds
Copy link
Collaborator

@iRedds iRedds commented Dec 28, 2021

Description
I believe there was a error during the design phase. In my opinion the error is that the typehint for the $options parameter in the route is nullable and has a default value of null.

In fact, the null value is not processed in any way, and the $options argument is used only as an array.

This PR changes the typehint and default value for the $options parameter.

- ?array $options = null
+ array $options = []
  • RouteCollection
  • RouteCollectionInterface

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@MGatner
Copy link
Member

MGatner commented Dec 28, 2021

While I am in favor of this change, we cannot update the interface without breaking all implementations of it elsewhere. This would be a good PR for version 5, when we are ready to roll up all these backwards-incompatible improvements.

@MGatner MGatner added breaking change Pull requests that may break existing functionalities next major version? Read this for a relevant v5 idea labels Dec 28, 2021
@iRedds
Copy link
Collaborator Author

iRedds commented Dec 28, 2021

This interface has only one class implementation. And I can hardly imagine a programmer who will purposefully pass null.

I have been hearing excuses about version 5 for over a year now, but there is not even a branch in the repository yet.
Maybe it's time?

@MGatner
Copy link
Member

MGatner commented Dec 29, 2021

I think calling them "excuses" is not fair. We follow semver which has very clear and strict guidelines on breaking changes. This makes a lot of things way cleaner and easier to handle, but complicates our jobs as framework developers: it is a trade-off.

I am not opposed to a conversation about "when to 5", but because major versions are our only chance to make breaking changes it will have to be the panned out very deliberately. This is also the time to make sweeping changes, which will require lots of dev time.

My opinion is that our roadmap still has unfinished business for version 4, and that the amount of time it has taken to address those means we are still short on contributors and therefor not ready to begin work on version 5 yet.

@iRedds
Copy link
Collaborator Author

iRedds commented Dec 29, 2021

I don't want to upset you but the team doesn't follow semver.
Semver is BC.Feature.Patch
But what is it really? Almost all features and fixes are published as patches. CIv4.2 switches to php-7.4 implementing language functions, and this is BC. Because there will be no backward compatibility.

If I am not mistaken, at the moment all the points of the roadmap have been fulfilled. The framework fulfills all the basic development needs. Any features can be added in minor versions.

I understand that the amount of work for v5 is large, but if you do nothing it will not decrease. But the enthusiasm of contributors can go away.

@kenjis
Copy link
Member

kenjis commented Dec 29, 2021

But what is it really? Almost all features and fixes are published as patches. CIv4.2 switches to php-7.4 implementing language functions, and this is BC. Because there will be no backward compatibility.

Yes, our rule is not Semver.

And where is the roadmap?
This? https://github.com/codeigniter4/CodeIgniter4/projects/3
It seems it is not updated.

If there is a real roadmap somewhere, it is better to show it.
Otherwise we cannot discuss about the roadmap.

And I think we can try creating 5.0 or 5.x branch and see how it goes.

@MGatner
Copy link
Member

MGatner commented Dec 29, 2021

We fully intend to follow semver. Maybe there is some quibbling over what "features" are, but the intent is to hold all new feature releases for the next minor release (major.minor.patch). For example, Publisher, Queue, Mailer are slotted for version 4.2 (even though I accidentally released Publisher already).

4.2 will not include breaking changes related to PHP 7.4; we will add 7.4-compatible language structures where they won't cause breaking changes. The minimum PHP version will increase from 7.3 to 7.4 but this is not a violation of semver since it does not affect the API, and Composer will handle the dependency calculation.

@MGatner
Copy link
Member

MGatner commented Dec 29, 2021

But the enthusiasm of contributors can go away.

I'm not sure what to say to this. I think this is a valid point, but the truth is also that we have a very, very small team of people doing the "necessary work" of the framework (fixing bugs, addressing issues, checking docs consistency, etc) which rarely comes from enthusiasm. We get a lot of "hey here's my great idea, I know I didn't follow the submission guidelines or write any tests, but you all can do that if you like my idea", which I'm grateful for the enthusiasm but it doesn't help with moving the framework forward.

(Just to be clear @iRedds I don't consider you in that category, your consistency, thoughtful input, and thorough PRs are very appreciated.)

@kenjis
Copy link
Member

kenjis commented Dec 30, 2021

If we fully follow semver, patch releases have only bug fixes.

CIv4.2 switches to php-7.4 implementing language functions, and this is BC. Because there will be no backward compatibility.

v4.2 is probably not BC in the sense of semver. If application code works without any change (only updates PHP version), it is not BC.

@iRedds
Copy link
Collaborator Author

iRedds commented Dec 30, 2021

I have looked at other frameworks and their versions coincide with my vision.
Symfony

  • 4.x - php 7.1 or higher
  • 5.x - php 7.2 or higher
  • 6.x - php 8.0 or higher

Laravel

  • 6.x - php 7.2 - 8.0
  • 8.x - php 7.3 - 8.1
  • 9.x - php 8.0 - 8.1

CakePHP

  • 2.x - php 5.3 - 7.1
  • 3.x - php 5.6 - 7.1
  • 4.x - php 7.2 - 8.1

By increasing the minimum php version, the major version of the framework is being increased.
Therefore, I assumed that 4.2 with 7.4 is BC.

@iRedds iRedds closed this Dec 30, 2021
@MGatner
Copy link
Member

MGatner commented Dec 30, 2021

I think that's a fine way to do it, but it is related to different choices. Each framework (and package for that matter) needs to decide:

  1. Will releases be scheduled to coincide with language versions?
  2. Will previous language versions be supported?

Both of these will determine whether/how long your software runs on unsupported language versions.

For CodeIgniter 4 we have decided not to support legacy PHP versions, at all. This was largely guided by available dev time, since tracking and maintaining multiple versions takes some work.

Semver ultimately only cares about your API, and relies on dependency management to handle dependencies. From the FAQ:

What should I do if I update my own dependencies without changing the public API?

That would be considered compatible since it does not affect the public API. Software that explicitly depends on the same dependencies as your package should have their own dependency specifications and the author will notice any conflicts.


I still think this is a good contribution, and the tags we've put on it makes sure it won't get lost when we work on version 5. @iRedds If you want to become a champion for version 5 please feel free to begin some conversations and the Slack and the forums - this is a community project and the maintainers will always heed the community.

@kenjis
Copy link
Member

kenjis commented Jan 9, 2022

@MGatner

We fully intend to follow semver.

My question is who wants it?

At least, there is no one yet who wants us to follow semver.
https://forum.codeigniter.com/thread-80892.html

@MGatner
Copy link
Member

MGatner commented Jan 9, 2022

I responded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Pull requests that may break existing functionalities next major version? Read this for a relevant v5 idea
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants