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

Added native return types #44

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pkruithof
Copy link

The ArrayAccess methods have native return types since PHP 7, which can result in deprecation messages like this (when using Symfony's debug classloader):

2022-05-24T14:21:41+00:00 [info] User Deprecated: Method "ArrayAccess::offsetUnset()" might add "void" as a native return type declaration in the future. Do the same in implementation "Dflydev\DotAccessData\Data" now to avoid errors or add an explicit @return annotation to suppress this message.

This PR adds native return types where possible.

{
return $this->has($key);
}

/**
* {@inheritdoc}
*
* @return mixed
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because mixed was added after PHP 7.1, it cannot be used as native return type yet.

Copy link
Contributor

@colinodell colinodell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no problem with these changes, but they do technically break BC (since this class is not final) and would therefore require a major version bump.

@pkruithof
Copy link
Author

I could remove the native return types and just add @return docblocks instead?

@pkruithof
Copy link
Author

@colinodell what do you propose?

@mutan
Copy link

mutan commented Sep 4, 2022

Hello!

The issue is still actual. Annoying notices after phpunit tests.

@simensen @colinodell what about @pkruithof proposal to add @return docblocks? Or any other solution.


  1x: Method "ArrayAccess::offsetExists()" might add "bool" as a native return type declaration in the future. Do the same in implementation "Dflydev\DotAccessData\Data" now to avoid errors or add an explicit @return annotation to suppress this message.

  1x: Method "ArrayAccess::offsetGet()" might add "mixed" as a native return type declaration in the future. Do the same in implementation "Dflydev\DotAccessData\Data" now to avoid errors or add an explicit @return annotation to suppress this message.

  1x: Method "ArrayAccess::offsetSet()" might add "void" as a native return type declaration in the future. Do the same in implementation "Dflydev\DotAccessData\Data" now to avoid errors or add an explicit @return annotation to suppress this message.

  1x: Method "ArrayAccess::offsetUnset()" might add "void" as a native return type declaration in the future. Do the same in implementation "Dflydev\DotAccessData\Data" now to avoid errors or add an explicit @return annotation to suppress this message.```

@madflow
Copy link
Contributor

madflow commented Oct 25, 2022

@pkruithof @colinodell @simensen

Can we kindly do something about these noisy messages in everybodies PHP 8.1 Unit Tests :D

I have created a quick "sister" PR: #45 . This should patch this issue, hopefully without a BC issue.

@colinodell
Copy link
Contributor

colinodell commented Oct 25, 2022

#45 seems like a sensible short-term approach as it silences these messages (which users shouldn't have to care about) without breaking BC. And merging #45 wouldn't prevent us from merging this one too (as a new major version) at some point.

@colinodell
Copy link
Contributor

#45 has been merged and released: https://github.com/dflydev/dflydev-dot-access-data/releases/tag/v3.0.2

@pkruithof
Copy link
Author

Cool, let me know when you are ready to merge this one, because I suspect I can change the mixed return type too then, since it would be a major version bump and thus probably remove PHP 7.1 support.

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.

None yet

4 participants