[BUGFIX] Create default Size with correct types in expandBackgroundShorthand

[oliverklee]

No description provided.

[coveralls]

coverage: 50.946%. remained the same
when pulling 06a7fad on bugfix/size-constructor-parameters
into 0c96d67 on main.

[JakeQZ]

We need some unit tests here to help us mere mortals understand what is happening, and what is supposed to be happening.

Once we've established what's going on, and how the original code worked (or didn't), the changelog entry can be more descriptive.

[oliverklee]

I agree that we should cover bug fixes (including fixes to issues found by PHPStan) with unit tests (i.e., regression tests).

In this particular case, I'd prefer not to spend time on writing tests as the offending code is part of a deprecated method that we're planning to remove in #511. Would you be okay with not having regression tests in this case?

This code would break once we add native types to constructors (see #818).

I'm not quite happy with the commit and changelog messages either and would appreciate some help.

This is the called constructor:

function __construct($fSize, $sUnit = null, $bIsColorComponent = false, $iLineNo = 0);

When constructing the Size instances, the offending code has an extraneous null argument before the $bIsColorComponent argument, which results in those constructor arguments:

• $fSize = 0 (correct)
• $sUnit = '%' (correct)
• $bIsColorComponent = null (should be false)
• $iLineNo = false (should be $this->iLineNo)
• $this->iLineNo going to nothing (extraneous constructor argument)

So in effect (as long as we don't have native type for the Size constructor`), the line number gets lost.

What could be a more fitting commit and change logmessage for this?

[JakeQZ]

In this particular case, I'd prefer not to spend time on writing tests as the offending code is part of a deprecated method that we're planning to remove in #511.

I hadn't relized this was in a deprecated method.

Would you be okay with not having regression tests in this case?

Yes, in this case.

When constructing the Size instances, the offending code has an extraneous null argument before the $bIsColorComponent argument, which results in ... the line number gets lost.

I think the line number being passed is not correct anyway, since it's that of the declaration block rather than the rule. Also, these are the default values for when background position is not specified, so perhaps it's right that no line number is set.

Thus it might be best to simply remove all the null/zero/false arguments (i.e. just have new Size(0, '%')), so that the behaviour is effectively unchanged (but the Size object will now be constructed with the correct/expected types).

What could be a more fitting commit and change logmessage for this?

Perhaps "Create default Size with correct types in expandBackgroundShorthand". But if this method is going to be removed anyway, and as it's already deprecated, perhaps it's best not to have a changelog entry - changes to deprecated methods aren't really relevant.

[oliverklee]

Thanks! I've updated the commit message and PR title accordingly and removed the changelog entry. (For the backport PR, I think the changelog entry would still be helpful.)

[JakeQZ]

I used the visual merge to resolve the conflict in phpstan-baseline.neon, which seems to have worked out OK.

Thus it might be best to simply remove all the null/zero/false arguments (i.e. just have new Size(0, '%')), so that the behaviour is effectively unchanged

I note you didn't adopt that suggestion, but as this is a deprecated method, it doesn't really matter.

(For the backport PR, I think the changelog entry would still be helpful.)

If we do a backport. But even then, the method is already deprecated in the 8.x branch.