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

Why is migration adapter nullable? #2200

Closed
martenb opened this issue May 30, 2023 · 2 comments · Fixed by #2261
Closed

Why is migration adapter nullable? #2200

martenb opened this issue May 30, 2023 · 2 comments · Fixed by #2261

Comments

@martenb
Copy link
Contributor

martenb commented May 30, 2023

I found this commit, where many methods changed their return type to nullable. f385bbb

For example AbstractMigration::getAdapter() returns null or AdapterInterface.
Is that even possible, that migration does not have adapter?

With static analysis (like phpstan) i must check everytime, that adapeter is not null...

assert($this->getAdapter() !== null);
$this->getAdapter()->...
@dereuromark
Copy link
Member

This most likely is due to legacy code expectations and can be fixed in a new major (1.0).

@MasterOdin
Copy link
Member

It's only really possible that getAdapter() returns null within the constructor as we don't pass in the adapter there. However, before we call the change, up, or down methods, we do setAdapter on the migration (and seed) so at that point, yeah, getAdapter() should always return something.

I think modifying the getAdapter() method to throw an exception if it's null should be fine to put in a minor release as while it's a "breaking change", the people it gets broken for would be those that are doing ->getAdapter() in a custom adapter, and then having it always return null which seems so unlikely to probably not matter.

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 a pull request may close this issue.

3 participants