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: Set font size to 10 when given 0 #2100

Merged
merged 1 commit into from
May 29, 2021
Merged

fix: Set font size to 10 when given 0 #2100

merged 1 commit into from
May 29, 2021

Conversation

drola
Copy link
Contributor

@drola drola commented May 15, 2021

This is:

- [x] a bugfix
- [ ] a new feature

Checklist:

Why this change is needed?

This change restored behavior from PHP7 in PHP8. In PHP7 calling
setSize(0) resulted in font size being set to 10. The fix addresses
change to equal comparisons in PHP8. Extra comparison is added to keep
result from PHP7 in PHP8 for the setSize(0) case.

@drola drola changed the title fix: Set font size to 10 when given 0 WIP fix: Set font size to 10 when given 0 May 16, 2021
@MarkBaker
Copy link
Member

Thank you for the contribution; although any fix to the values passed to the method should include other invalid values as well

@drola
Copy link
Contributor Author

drola commented May 26, 2021

Thank you for the review.

I expanded the test and added the handling of all other invalid values.

Copy link
Member

@MarkBaker MarkBaker left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes to full validate the value, and for ensuring that it's covered by unit tests...

When we first wrote those basic setters, we never really expected any developer to pass through invalid values, so the validation checks were minimal.... over the years, we've been proven wrong time and again, but never really got round to improving the validation checks, as there were always more important demands for more serious bugs or new features

@@ -255,9 +255,16 @@ public function getSize()
*/
public function setSize($pValue)
Copy link
Member

Choose a reason for hiding this comment

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

The docblock needs modifying to mixed to keep phpstan happy, and it looks like phpcs is complaining about trailing white space in the comment lines.
If you can just make those changes, then the PR is good to merge

Copy link
Member

@MarkBaker MarkBaker left a comment

Choose a reason for hiding this comment

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

The docblock needs modifying to mixed to keep phpstan happy, and it looks like phpcs is complaining about trailing white space in the comment lines.
If you can just make those changes, then the PR is good to merge

This change restored behavior from PHP7 in PHP8. In PHP7 calling
setSize(0) resulted in font size being set to 10. The fix addresses
change to equal comparisons in PHP8. Extra comparison is added to keep
result from PHP7 in PHP8 for the setSize(0) case.
@drola
Copy link
Contributor Author

drola commented May 29, 2021

Thank you. Updated the docblock, ran composer fix, composer check and rebased on top of master.

@MarkBaker MarkBaker merged commit 0b0f022 into PHPOffice:master May 29, 2021
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