Added return types #1439
Added return types #1439
Conversation
There was a problem hiding this comment.
thanks for the pull request.
as we are doing a new major version, i think that we should also change the LoaderInterface to have typed parameters (and then adjust all implementations accordingly)
i want to challenge if we need to make return types nullable - i would prefer to go the other direction of making more things not nullable.
stephanvierkant
force-pushed
the
types
branch
2 times, most recently
from
2078a4f
to
00e83d0
Compare
|
@stephanvierkant do you plan to follow up on the feedback? i would love to get these improvements in once we figured out the remaining questions. |
| @@ -56,7 +56,7 @@ public function remove(array $paths, array $filters): void | |||
| $this->resolver->remove($paths, $filters); | |||
| } | |||
|
|
|||
| protected function rewriteUrl(string $url): string | |||
| protected function rewriteUrl(string $url): ?string | |||
There was a problem hiding this comment.
the only way how this can return null is when there is a regex error in the preg_replace call. is the calling side of this handling that? if not, i'd prefer to check the return value of preg_replace in this method and use preg_last_error_msg to throw an exception. then we can leave this as returning a non-nullable string.
| * Returns resource url. | ||
| */ | ||
| public function getUrl(): ?string | ||
| public function getUrl(): string |
There was a problem hiding this comment.
i don't think you can change this to non-nullable as the url property of the class is nullable.
There was a problem hiding this comment.
I've just looked at ‘protected ?string $url;’
There was a problem hiding this comment.
the correct return type should look like this:
| public function getUrl(): string | |
| public function getUrl(): ?string |
|
Any news ? |
|
i would love to get this moving again, but have little time. @cbeyer if you want to pick up the branch from stephan and do a new pull request with the conflicts resolved and possibly more strict typing cleanup, please do and i will be happy to review the changes. |
|
@tacman this would be a crucial bit to modernize the code for 3.x |
Added or changed some return types. Most of the methods already had a return type. I couldn't find any methods where a parameter type declaration could be added.