Amend type hint to match default value

[JoshuaBehrens]

1. Why is this change necessary?

The phpdocs for the route defaults does not match the typehint in the arguments. This results in an an exception on the php side as the type constraint is not fulfilled. So the validity check whether the parameter is ok is never reached and no readable exception is thrown/logged.

2. What does this change do, exactly?

Loosen up the typehint by adding nullability.

3. Describe each step to reproduce the issue or behaviour.

1. Do not configure contact layout page
2. Click on contact in storefront
3. Do not get a contact form
4. Look in logs
5. Uncaught PHP Exception TypeError: "Argument 1 passed to Shopware\Storefront\Controller\CmsController::page() must be of the type string, null given
6. Expecting Parameter "Parameter id missing" is missing.
7. Wut?
8. Check message concatenation for MissingRequestParameterException
9. See weird refactoring result
10. Take notes for next pull request

4. Checklist

• I have written tests and verified that they fail without my change
• I have squashed any insignificant commits
• I have written or adjusted the documentation according to my changes
• This change has comments for package types, values, functions, and non-obvious lines of code
• I have read the contribution requirements and fulfil them.

[JoshuaBehrens]

There are more cases (even in the same controller) that suffer from this. I can resolve all of them. I just want to point onto this and listen whether this is a needed and viable fix for the other cases.

[keulinho]

IMHO the null default doesn't make sense why allow null on the framework level and then having to handle null in userland?

Also shouldn't this be catchable by static analysis? as the if block is unreachable, would be nice to prevent this issues in the future

[JoshuaBehrens]

I agree. You use psalm, don't you? I can imagine psalm is catching this.

[mitelg]

probably not on the low level, we have set up 🙈

[JoshuaBehrens]

If one remove the check in the userland should I add a uuid check on id instead?

[keulinho]

PhpStan has an UnreachableIfBranchRule, that should be activated but somehow misses this, probably because an empty string would also be falsy

[JoshuaBehrens]

I applied the requested changes

[shopwareBot]

Hello,

thank you for creating this pull request.
I have opened an issue on our Issue Tracker for you. See the issue link: https://issues.shopware.com/issues/NEXT-7907

Please use this issue to track the state of your pull request.

[pantrtxp]

@JoshuaBehrens can you please resolve the conflicts here 🙂 ?

[JoshuaBehrens]

I put it on my TODO 👍 👀

[JoshuaBehrens]

✔️

[pantrtxp]

Hi @JoshuaBehrens

The TravisCI is finally working again 🚀 but it looks like some test are failing ☹️ could you plase have a look at them

Sunny regards
Pantrtxp

[pantrtxp]

Hi @JoshuaBehrens
This PR is still incomplete.
Will you please update this PR?

Sunny regards
Pantrtxp

[JoshuaBehrens]

I can retry testing it but the last time I tried my tests were not working because of a missing google client dependency.

[tobiasberge]

Hi @JoshuaBehrens,

are those changes not the same as in this PR: https://github.com/shopware/platform/pull/471/files besides the route annotations?

Best regards
Tobias

[mitelg]

@tobiasberge @JoshuaBehrens I guess there are a few commits to much 😉 needs a clean-up

[JoshuaBehrens]

Damn it I guess I rebased onto the wrong branch :D

[tobiasberge]

@JoshuaBehrens Do you still have issues with the Google client dependency?

[JoshuaBehrens]

I resolved it on the boostday 😁

[tobiasberge]

Hi @JoshuaBehrens,

there are still the changes from the other PR (EntityCollection class) in this branch.
Could you please update the branch?

Best regards
Tobias

[JoshuaBehrens]

Sorry for delaying. I try to get it into my schedule ASAP

[JoshuaBehrens]

Hey @tobiasberge I tidied things a bit up

[JoshuaBehrens]

Thanks for merging 👍 💙

[J-Rahe]

Hi @JoshuaBehrens ,
thanks for your contribution!