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

Fix return annotation type #7

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix return annotation type #7

wants to merge 3 commits into from

Conversation

jbtcd
Copy link

@jbtcd jbtcd commented Aug 6, 2024

PR Description

After I updated the package in a Spryker application, PHPStan informed me that the return type does not match the one from the Spryker project.

As the format of the array is described in the project itself, I want to update the annotation to the correct type.

# src/Spryker/Zed/Propel/Business/Model/Schema/Validator/PropelSchemaValidator.php
...
    /**
     * Format:
     * ```
     * [
     *     'foo_bar.schema.xml' => [
     *         'attribute_a',
     *         'attribute_b',
     *     ]
     * ]
     * ```
     *
     * @var array
     */
    protected $whiteListedTableAttributes;
...

The updated docblock from the getWhitelistForAllowedAttributeValueChanges() method will match these array format.

At least the docblock fix should make it into the master branch. The typhint is a nice-to-have in my opinion.

Checklist

  • I agree with the Code Contribution License Agreement in CONTRIBUTING.md

@gechetspr
Copy link
Contributor

Dear contributor,

Your pull request will be reviewed by our team. In the meantime, please ensure that the PR complies with Spryker's coding guidelines and that your changes are covered by tests.

Thank you very much for your contribution to the development of Spryker and the growth of the Spryker community!

*/
public function getWhitelistForAllowedAttributeValueChanges()
public function getWhitelistForAllowedAttributeValueChanges(): array
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey! Unfortunately we can't allow this change as it may be considered as a BC break (public api method definition should not be changed in a non major release even if the same type was defined in doc blocks).
BC breaks force us to make a major release and this change is not major enough to do so.

I would recommend to just remove typehint

Copy link
Author

Choose a reason for hiding this comment

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

Hi! Yes, that was already my guess in the initials PR description. I have removed the typehint and now only the DocBlock change is in the PR.
Thanks for your feedback!

Copy link
Author

Choose a reason for hiding this comment

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

Hey @gechetspr, do I have to do anything else for the PR to be accepted?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey! Looks like I need a tune a bit our tooling to send auto notification when things are released.

We released your fix in https://api.release.spryker.com/release-group/5512
Thank you! And sorry for the late notification

@jbtcd jbtcd changed the title Add array as the return type and fix the return annotation type Fix the return annotation type Sep 11, 2024
@jbtcd jbtcd changed the title Fix the return annotation type Fix return annotation type Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants