Skip to content

PHP 8 compatibility. #843

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.

Sign up for GitHub

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
wants to merge 2 commits into from
Closed

PHP 8 compatibility. #843

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
887ee0e
PHP 8 compatibility.
glodov Jan 25, 2023
322cb9e
PHP 8 compatibility.
Mar 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/Image/Point.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,12 @@ final class Point implements PointInterface
*/
public function __construct($x, $y)
{
if (!\is_int($x)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we pass wrong arguments, they will be silently converted to 0.

I'd instead do something like

        if (!is_numeric($x)) {
            throw new InvalidArgumentException('x must be numeric');
        }
        $x = (int) $x;
        if (!is_numeric($y)) {
            throw new InvalidArgumentException('y must be numeric');
        }
        $y = (int) $y;

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think for better consistency, this should be handled the same way as Box class does:

Imagine/src/Image/Box.php

Lines 41 to 46 in c10ea7f

if (!\is_int($width)) {
$width = (int) round($width);
}
if (!\is_int($height)) {
$height = (int) round($height);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

With the Box class it's a bit easier, since it can't accept a width or a height with a value of 0 (so, an exception will be thrown if we pass a non-numeric value to it).
But I do agree that we should use round.

So, what about something like this?

if (!is_numeric($x)) {
    throw new InvalidArgumentException('x must be numeric');
}
$x = (int) round((float) $x));

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I didn’t think of the 0 case.
So +1 for you latest code suggestion 👍

$x = (int) round($x);
}
if (!\is_int($y)) {
$y = (int) round($y);
}
if ($x < 0 || $y < 0) {
throw new InvalidArgumentException(sprintf('A coordinate cannot be positioned outside of a bounding box (x: %s, y: %s given)', $x, $y));
}
Expand Down