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

Use the ramsey/composer-install action to install dependencies #262

Merged
merged 3 commits into from
Dec 11, 2020
Merged

Use the ramsey/composer-install action to install dependencies #262

merged 3 commits into from
Dec 11, 2020

Conversation

nicwortel
Copy link
Contributor

Follow-up of doctrine/.github#16.

Please note: the contributing guidelines of this repo state that you only accepts PR's to master, but the last 5 PR's (#261, #259, #258, #257, #256) were merged into the default branch (1.6.x). Is the CONTRIBUTING.md file outdated?

@nicwortel
Copy link
Contributor Author

How would you like me to handle the failing build? I see it was already failing on the default branch, but I would like to fix it in this PR so you don't have to merge a PR with a failing build.

Psalm seems to fail because of a "redundant" type cast:

    /**
     * Set the number of first result that this Criteria should return.
     *
     * @param int|null $firstResult The value to set.
     *
     * @return Criteria
     */
    public function setFirstResult($firstResult)
    {
        $this->firstResult = $firstResult === null ? null : (int) $firstResult;

        return $this;
    }

I don't think it's possible to add a native type hint to the method parameter without breaking backward compatibility, and it seems like a good idea to force the value to int in case another type is provided. So I think the best solution would be to suppress RedundantCastGivenDocblockType errors, do you agree?

@nicwortel nicwortel marked this pull request as ready for review December 10, 2020 21:20
@greg0ire
Copy link
Member

Is the CONTRIBUTING.md file outdated?

Uuuuh yes I think so, we have been migrating to merging up on all repositories, no exceptions. I think we should replace the contents of that file with a link to https://www.doctrine-project.org/contribute/index.html

@greg0ire
Copy link
Member

it seems like a good idea to force the value to int in case another type is provided.

Although the phpdoc is not as strong a contract as a type declaration, it's still a contract IMO, and I would therefore just drop that cast. Unless that phpdoc has recently been added, a change in behavior for the case where you provide something else would not be a BC-break, because that behavior is undefined.

@nicwortel
Copy link
Contributor Author

it seems like a good idea to force the value to int in case another type is provided.

Although the phpdoc is not as strong a contract as a type declaration, it's still a contract IMO, and I would therefore just drop that cast. Unless that phpdoc has recently been added, a change in behavior for the case where you provide something else would not be a BC-break, because that behavior is undefined.

OK, I have removed the type cast 👍

@greg0ire greg0ire added this to the 1.6.8 milestone Dec 11, 2020
@greg0ire greg0ire merged commit 1e88553 into doctrine:1.6.x Dec 11, 2020
@greg0ire
Copy link
Member

Thanks @nicwortel !

@nicwortel nicwortel deleted the ramsey-composer-install branch December 11, 2020 18:17
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.

2 participants