-
Notifications
You must be signed in to change notification settings - Fork 9.4k
code refactor, php8.x adjustmentts, param types, etc #39868
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
base: 2.4-develop
Are you sure you want to change the base?
code refactor, php8.x adjustmentts, param types, etc #39868
Conversation
Hi @jakwinkler. Thank you for your contribution!
Allowed build names are:
You can find more information about the builds here For more details, review the Code Contributions documentation. |
@magento run all tests |
1 similar comment
@magento run all tests |
@magento run all tests |
@magento create issue |
@jakwinkler, I do agree with these changes in general, but they most likely will break anyone who extended from these interfaces or classes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates several Magento EAV classes and interfaces to use PHP 8 type declarations and return types, and adjusts docblocks for consistency.
- Added scalar and union type hints to methods (e.g.,
string
,int
,string|int
,array
,bool
) - Updated return type declarations across repository and management classes/interfaces
- Refined docblocks and imports to match new signatures
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
app/code/Magento/Eav/Model/Entity/Attribute/OptionManagement.php | Added parameter and return type hints; adjusted docblocks |
app/code/Magento/Eav/Model/AttributeRepository.php | Introduced method return types and scalar type hints; imports |
app/code/Magento/Eav/Api/AttributeRepositoryInterface.php | Updated interface method signatures with type hints and return types |
app/code/Magento/Eav/Api/AttributeOptionManagementInterface.php | Added scalar and union types in method signatures and docblocks |
Comments suppressed due to low confidence (1)
app/code/Magento/Eav/Api/AttributeOptionManagementInterface.php:40
- The
getItems
method may throwNoSuchEntityException
when the attribute does not exist (vialoadAttribute
). Consider adding@throws NoSuchEntityException
to the docblock for completeness.
* @throws \Magento\Framework\Exception\NoSuchEntityException
@@ -226,7 +228,7 @@ public function getItems($entityType, $attributeCode) | |||
* @return void | |||
* @throws NoSuchEntityException | |||
*/ | |||
protected function validateOption($attribute, $optionId) | |||
protected function validateOption(EavAttributeInterface $attribute, int $optionId): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The public delete method accepts string|int $optionId
but validateOption
is declared to accept only int
, which may lead to type mismatches under strict mode. Consider updating validateOption
to accept string|int
and cast to int
internally, or enforce int
at the public API boundary.
Copilot uses AI. Check for mistakes.
@jakwinkler, please review the comments from Copilot and let us know if you would like anything to be fixed based on them. If no, I'll review the pull request |
Description (*)
Just some type fixes + upgrade to php8.x
Needs to be tested and validated properly, as some method properties still do not match in core.
I've made this fix due to this code:
as it suppose to be an integer, but string has to be passed as the
$1
parameterContribution checklist (*)
Resolved issues: