Skip to content

Rework non-empty-string assertions #86

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

Closed

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented Jan 10, 2022

This does a couple of things

  • Removes the hacky non-empty-string expression re-usage and replaces it with type specifying and calls to $this->typeSpecifier->create (I hope this closes False positive on Assert::upper() always evaluates as true #85)
  • Removes the expression adaption for nullOr* assertions and replaces it with TypeCombinator::addNull and calls to $this->typeSpecifier->create. This should close Assert::nullOrStringNotEmpty() with string|null will always evaluate to true #33.
  • Tries to detect the type from SpecifiedTypes a bit smarter by also looking at the sureNotTypes. This retains non-empty-string via nullOr* and all* better
  • Adds integration and many other tests, e.g. some cases of all* and nullOr* almost everywhere, where it makes sense
  • Fixes some of the non-empty-string resulting assertions that expect a string as argument (e.g. contains or notWhitespaceOnly)

I know this is a big one, I'm happy to split it up or whatever makes sense. Some of the non-empty-string things seem to be a bit intertwined though.

@herndlm herndlm force-pushed the rework-non-empty-string-assertions branch 10 times, most recently from 4db13dc to 805c1b5 Compare January 11, 2022 08:06
@VincentLanglet
Copy link

Thanks for the pretty quick fix.

@herndlm
Copy link
Contributor Author

herndlm commented Jan 15, 2022

@VincentLanglet are you affected by a bug, e.g. #85? Maybe you can help me reproducing it or adding a regression test :)

@VincentLanglet
Copy link

@VincentLanglet are you affected by a bug, e.g. #85? Maybe you can help me reproducing it or adding a regression test :)

The following code

        Assert::stringNotEmpty($url);
        Assert::contains($url, '/'); // => Always true
        Assert::startsWith($url, 'https://github.com/');  // => Always true

report two errors (with the phpstan strict plugin).

@herndlm herndlm force-pushed the rework-non-empty-string-assertions branch from 805c1b5 to 2e817b6 Compare January 15, 2022 13:03
@herndlm herndlm marked this pull request as draft January 15, 2022 13:37
@herndlm herndlm force-pushed the rework-non-empty-string-assertions branch from 2e817b6 to 7f2fbf1 Compare January 15, 2022 13:37
@herndlm
Copy link
Contributor Author

herndlm commented Jan 15, 2022

Ok, I think I was successful in adding the regressiontest by using strict-rules. But it shows that the problem still persists, so I'm kind of stuck.
This PR then fixes/improves some nullOr* all* assertions and does some refactoring I guess, but it does not fix the referenced issues unfortunately. They seem to be coming from phpstan's ImpossibleCheckTypeStaticMethodCallRule and I'm not sure yet if they make sense or not. Might be a incompatibility with non-empty-string? Maybe Ondrej can help out, sorry..

From PHPStan's point of view, based on types, it makes sense I guess to think that e.g. upper does not add additional type value to something that is already non-empty-string because e.g. there was a "length is 3" check before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants