Update zendframework dependencies #8938
Conversation
…d-code now generates them
…d-code now generates them
…now generates them
| @@ -228,7 +228,6 @@ protected function _getGetMethod() | |||
| 'parameters' => [ | |||
| [ | |||
| 'name' => 'id', | |||
| 'type' => 'int', | |||
There was a problem hiding this comment.
This will cause an fatal error if someone uses type hinting in a repository interface.
There was a problem hiding this comment.
How?
As stated in the commits this type configuration was completely ignored in zend-code 2.x see: https://github.com/zendframework/zend-code/blob/master/doc/book/migration.md#string-int-float-bool-are-no-longer-ignored. So removing this should have no affect at all as it didn't do anything in the first place. This now generates scalar type hints in 3.x.
There was a problem hiding this comment.
Yes, Zend Code 3.x uses type to generate type hinting. That's why if someone writes get method in a repository interface with type hinting (public function get(int $id)) it will cause the fatal error because generated repository will be inconsistent to the interface. Your changes do not affect Magento core code (because it does not use type hinting) but affect third-party developers, who will use PHP 7.0 features.
There was a problem hiding this comment.
As an example, you can write some repository interface with type hinting, specify preference via DI.xml to an auto-generated instance and run setup:di:compile command.
I'm also working on this task and I've used Reflection to solve this issue, it allows to get correct argument type of method.
There was a problem hiding this comment.
Ah yeah makes sense. I was more trying to just get the dependencies updated and not introduce new features as this fatal would still occur now surely. I think that should be tackled with a further PR - do you agree?
There was a problem hiding this comment.
I think that should be tackled with a further PR - do you agree?
Do you mean in the separated PR?
I think it should be done in the scope of one PR and covered with additional tests because existing tests are not enough.
There was a problem hiding this comment.
Fair enough - I'm not really interested in working on that so should I just close this PR? I wanted the dependencies updated as we are experiencing many clashes with other packages we want to use and develop with due to these old and tight constraints.
There was a problem hiding this comment.
Fair enough - I'm not really interested in working on that so should I just close this PR?
It's up to you, in my opinion, this PR need to finalize. But anyway, you have done a good job and this task has very high priority for us because allows supporting PHP 7.x features out of the box and outdated dependencies are always a headache.
Description
Updated zendframework dependencies and fixed any BC breaks. There still might be a few issues which need resolving.
This is to make sure we can have latest versions and be more flexible with constraints. This allows us to have more secure dependencies and also be able to install dependencies which rely on newer zf packages. Also paves the way for PHP7 & 7.1 code generation.
Manual testing scenarios
I guess we need to check that everything still works. Hopefully the unit tests should have that covered. That's help me fix the majority of issues. I've navigated around the system and it seems to work fine.
Contribution checklist