Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
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
2 changes: 2 additions & 0 deletions docs/AVAILABLE-TYPES.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,5 @@
| cidr[] | _cidr | `MartinGeorgiev\Doctrine\DBAL\Types\CidrArray` |
| macaddr | macaddr | `MartinGeorgiev\Doctrine\DBAL\Types\Macaddr` |
| macaddr[] | _macaddr | `MartinGeorgiev\Doctrine\DBAL\Types\MacaddrArray` |
| point | point | `MartinGeorgiev\Doctrine\DBAL\Types\Point` |
| point[] | _point | `MartinGeorgiev\Doctrine\DBAL\Types\PointArray` |
7 changes: 7 additions & 0 deletions docs/INTEGRATING-WITH-DOCTRINE.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ Type::addType('inet', "MartinGeorgiev\\Doctrine\\DBAL\\Types\\Inet");
Type::addType('inet[]', "MartinGeorgiev\\Doctrine\\DBAL\\Types\\InetArray");
Type::addType('macaddr', "MartinGeorgiev\\Doctrine\\DBAL\\Types\\Macaddr");
Type::addType('macaddr[]', "MartinGeorgiev\\Doctrine\\DBAL\\Types\\MacaddrArray");

Type::addType('point', "MartinGeorgiev\\Doctrine\\DBAL\\Types\\Point");
Type::addType('point[]', "MartinGeorgiev\\Doctrine\\DBAL\\Types\\PointArray");
```


Expand Down Expand Up @@ -204,6 +207,10 @@ $platform->registerDoctrineTypeMapping('inet[]','inet[]');
$platform->registerDoctrineTypeMapping('_inet','inet[]');
$platform->registerDoctrineTypeMapping('macaddr[]','macaddr[]');
$platform->registerDoctrineTypeMapping('_macaddr','macaddr[]');

$platform->registerDoctrineTypeMapping('point','point');
$platform->registerDoctrineTypeMapping('point[]','point[]');
$platform->registerDoctrineTypeMapping('_point','point[]');
...

```
13 changes: 13 additions & 0 deletions docs/INTEGRATING-WITH-LARAVEL.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ return [
'macaddr' => 'macaddr',
'macaddr[]' => 'macaddr[]',
'_macaddr' => 'macaddr[]',

'point' => 'point',
'point[]' => 'point[]',
'_point' => 'point[]',
],
],
],
Expand Down Expand Up @@ -80,6 +84,9 @@ return [
'inet[]' => MartinGeorgiev\Doctrine\DBAL\Types\InetArray::class,
'macaddr' => MartinGeorgiev\Doctrine\DBAL\Types\Macaddr::class,
'macaddr[]' => MartinGeorgiev\Doctrine\DBAL\Types\MacaddrArray::class,

'point' => MartinGeorgiev\Doctrine\DBAL\Types\Point::class,
'point[]' => MartinGeorgiev\Doctrine\DBAL\Types\PointArray::class,
],
];
```
Expand Down Expand Up @@ -301,6 +308,12 @@ class DoctrineEventSubscriber implements Subscriber
if (!Type::hasType('macaddr[]')) {
Type::addType('macaddr[]', \MartinGeorgiev\Doctrine\DBAL\Types\MacaddrArray::class);
}
if (!Type::hasType('point')) {
Type::addType('point', \MartinGeorgiev\Doctrine\DBAL\Types\Point::class);
}
if (!Type::hasType('point[]')) {
Type::addType('point[]', \MartinGeorgiev\Doctrine\DBAL\Types\PointArray::class);
}
}
}
```
Expand Down
7 changes: 7 additions & 0 deletions docs/INTEGRATING-WITH-SYMFONY.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ doctrine:
inet[]: MartinGeorgiev\Doctrine\DBAL\Types\InetArray
macaddr: MartinGeorgiev\Doctrine\DBAL\Types\Macaddr
macaddr[]: MartinGeorgiev\Doctrine\DBAL\Types\MacaddrArray

point: MartinGeorgiev\Doctrine\DBAL\Types\Point
point[]: MartinGeorgiev\Doctrine\DBAL\Types\PointArray
```


Expand Down Expand Up @@ -73,6 +76,10 @@ doctrine:
macaddr: macaddr
macaddr[]: macaddr[]
_macaddr: macaddr[]

point: point
point[]: point[]
_point: point[]
```


Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<?php

declare(strict_types=1);

namespace MartinGeorgiev\Doctrine\DBAL\Types\Exceptions;

use Doctrine\DBAL\Types\ConversionException;

/**
* @since 3.1
*
* @author Sébastien Jean <sebastien.jean76@gmail.com>
*/
class InvalidPointArrayItemForDatabaseException extends ConversionException
{
private static function create(string $message, mixed $value): self
{
return new self(\sprintf($message, \var_export($value, true)));
}

public static function isNotAPoint(mixed $value): self
{
return self::create('Given value of %s is not a point.', $value);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
<?php

declare(strict_types=1);

namespace MartinGeorgiev\Doctrine\DBAL\Types\Exceptions;

use Doctrine\DBAL\Types\ConversionException;

/**
* @since 3.1
*
* @author Sébastien Jean <sebastien.jean76@gmail.com>
*/
class InvalidPointArrayItemForPHPException extends ConversionException
{
private static function create(string $message, mixed $value): self
{
return new self(\sprintf($message, \var_export($value, true)));
}

public static function forInvalidType(mixed $value): self
{
return self::create('Array values must be strings, %s given', $value);
}

public static function forInvalidFormat(mixed $value): self
{
return self::create('Invalid point format in array: %s', $value);
}

public static function forInvalidArrayType(mixed $value): self
{
return self::create('Value must be an array, %s given', $value);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<?php

declare(strict_types=1);

namespace MartinGeorgiev\Doctrine\DBAL\Types\Exceptions;

use Doctrine\DBAL\Types\ConversionException;

/**
* @since 3.1
*
* @author Sébastien Jean <sebastien.jean76@gmail.com>
*/
class InvalidPointForDatabaseException extends ConversionException
{
private static function create(string $message, mixed $value): self
{
return new self(\sprintf($message, \var_export($value, true)));
}

public static function forInvalidType(mixed $value): self
{
return self::create('Database value must be a string, %s given', $value);
}

public static function forInvalidFormat(mixed $value): self
{
return self::create('Invalid point format in database: %s', $value);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<?php

declare(strict_types=1);

namespace MartinGeorgiev\Doctrine\DBAL\Types\Exceptions;

use Doctrine\DBAL\Types\ConversionException;

/**
* @since 3.1
*
* @author Sébastien Jean <sebastien.jean76@gmail.com>
*/
class InvalidPointForPHPException extends ConversionException
{
private static function create(string $message, mixed $value): self
{
return new self(\sprintf($message, \var_export($value, true)));
}

public static function forInvalidType(mixed $value): self
{
return self::create('Value must be a point, %s given', $value);
}
}
53 changes: 53 additions & 0 deletions src/MartinGeorgiev/Doctrine/DBAL/Types/Point.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
<?php

declare(strict_types=1);

namespace MartinGeorgiev\Doctrine\DBAL\Types;

use Doctrine\DBAL\Platforms\AbstractPlatform;
use MartinGeorgiev\Doctrine\DBAL\Types\Exceptions\InvalidPointForDatabaseException;
use MartinGeorgiev\Doctrine\DBAL\Types\Exceptions\InvalidPointForPHPException;
use MartinGeorgiev\Doctrine\DBAL\Types\ValueObject\Point as PointValueObject;

/**
* Implementation of PostgreSQL POINT data type.
*
* @see https://www.postgresql.org/docs/17/datatype-geometric.html#DATATYPE-GEOMETRIC-POINTS
* @since 3.1
*
* @author Sébastien Jean <sebastien.jean76@gmail.com>
*/
class Point extends BaseType
{
protected const TYPE_NAME = 'point';

public function convertToDatabaseValue($value, AbstractPlatform $platform): ?string
{
if ($value === null) {
return null;
}

if (!$value instanceof PointValueObject) {
throw InvalidPointForPHPException::forInvalidType($value);
}

return (string) $value;
}

public function convertToPHPValue($value, AbstractPlatform $platform): ?PointValueObject
{
if ($value === null) {
return null;
}

if (!\is_string($value)) {
throw InvalidPointForDatabaseException::forInvalidType($value);
}

try {
return PointValueObject::fromString($value);
} catch (\InvalidArgumentException) {
throw InvalidPointForDatabaseException::forInvalidFormat($value);
}
}
}
82 changes: 82 additions & 0 deletions src/MartinGeorgiev/Doctrine/DBAL/Types/PointArray.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
<?php

declare(strict_types=1);

namespace MartinGeorgiev\Doctrine\DBAL\Types;

use MartinGeorgiev\Doctrine\DBAL\Types\Exceptions\InvalidPointArrayItemForDatabaseException;
use MartinGeorgiev\Doctrine\DBAL\Types\Exceptions\InvalidPointArrayItemForPHPException;
use MartinGeorgiev\Doctrine\DBAL\Types\ValueObject\Point as PointValueObject;

/**
* Implementation of PostgreSQL POINT[] data type.
*
* @see https://www.postgresql.org/docs/17/datatype-geometric.html#DATATYPE-GEOMETRIC-POINTS
* @since 3.1
*
* @author Sébastien Jean <sebastien.jean76@gmail.com>
*/
class PointArray extends BaseArray
{
protected const TYPE_NAME = 'point[]';

protected function transformArrayItemForPostgres(mixed $item): string
{
if (!$item instanceof PointValueObject) {
throw InvalidPointArrayItemForDatabaseException::isNotAPoint($item);
}

return '"'.$item.'"';
}

protected function transformPostgresArrayToPHPArray(string $postgresArray): array
{
if (!\str_starts_with($postgresArray, '{"') || !\str_ends_with($postgresArray, '"}')) {
return [];
}

$trimmedPostgresArray = \mb_substr($postgresArray, 2, -2);
if ($trimmedPostgresArray === '') {
return [];
}

return \explode('","', $trimmedPostgresArray);
}

public function transformArrayItemForPHP(mixed $item): ?PointValueObject
{
if ($item === null) {
return null;
}

if (!\is_string($item)) {
$this->throwInvalidTypeException($item);
}

try {
return PointValueObject::fromString($item);
} catch (\InvalidArgumentException) {
$this->throwInvalidFormatException($item);
}
}

public function isValidArrayItemForDatabase(mixed $item): bool
{
return $item instanceof PointValueObject;
}

protected function throwInvalidTypeException(mixed $value): never
{
throw InvalidPointArrayItemForPHPException::forInvalidType($value);
}

protected function throwInvalidFormatException(mixed $value): never
{
throw InvalidPointArrayItemForPHPException::forInvalidFormat($value);
}

protected function throwInvalidItemException(): never
{
throw InvalidPointArrayItemForPHPException::forInvalidFormat('Array contains invalid point items');
}
}
60 changes: 60 additions & 0 deletions src/MartinGeorgiev/Doctrine/DBAL/Types/ValueObject/Point.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
<?php

declare(strict_types=1);

namespace MartinGeorgiev\Doctrine\DBAL\Types\ValueObject;

/**
* @since 3.1
*
* @author Sébastien Jean <sebastien.jean76@gmail.com>
*/
final class Point implements \Stringable
{
public const POINT_REGEX = '/\((-?\d+(?:\.\d{1,6})?),\s*(-?\d+(?:\.\d{1,6})?)\)/';

public function __construct(
Copy link
Owner

Choose a reason for hiding this comment

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

@coderabbitai, suggest a commitable comment on how POINT_REGEX can be used here to prevent incorrect object instantiation (e.g. when the precision is beyond the maximum allowed one for the said regex validation. Consider memory implications.

Copy link

Choose a reason for hiding this comment

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

@martin-georgiev The regex is currently defined but not utilized within the Point class itself. Here's a suggested comment explaining how it could be used to validate object creation:

/**
 * The POINT_REGEX constant defines valid PostgreSQL POINT format with up to 6 decimal places.
 * 
 * Note: This class currently doesn't validate input precision during construction.
 * To ensure consistency with the regex validation used elsewhere, consider:
 * 1. Adding validation in the constructor to round or truncate coordinates to 6 decimal places
 * 2. Or implementing a named constructor that validates string input against POINT_REGEX
 * 
 * Example implementation:
 * public function __construct(float $x, float $y)
 * {
 *     // Round to 6 decimal places to match regex precision
 *     $this->x = round($x, 6);
 *     $this->y = round($y, 6);
 * }
 * 
 * While individual high-precision floats have minimal memory impact,
 * consistent precision handling prevents potential issues with serialization
 * and ensures string parsing/formatting consistency.
 */
public const POINT_REGEX = '/\((-?\d+(?:\.\d{1,6})?),\s*(-?\d+(?:\.\d{1,6})?)\)/';

Copy link
Owner

Choose a reason for hiding this comment

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

Suggest how the constructor will throw on attempts to instantiate an object that will not allow the value object to resolve to a compatible value as stipulated by the regex.

Copy link

Choose a reason for hiding this comment

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

@martin-georgiev Here's a suggestion for how the constructor could throw an exception when input values would create an object incompatible with the regex pattern:

public function __construct(float $x, float $y)
{
    // Validate that the coordinates don't exceed the 6 decimal places allowed by POINT_REGEX
    $xString = (string)$x;
    $yString = (string)$y;
    
    // Check if the string representation has more than 6 decimal places
    if (preg_match('/\.\d{7,}$/', $xString) || preg_match('/\.\d{7,}$/', $yString)) {
        throw new \InvalidArgumentException(
            sprintf('Point coordinates must not exceed 6 decimal places. Given x: %s, y: %s', $xString, $yString)
        );
    }
    
    $this->x = $x;
    $this->y = $y;
}

This ensures that attempting to create a Point with values that would later fail the regex validation will immediately throw an exception, maintaining consistency between object creation and validation.

You could also consider introducing a static factory method for creating points from strings, which would validate directly against the regex:

public static function fromString(string $pointString): self
{
    if (!preg_match(self::POINT_REGEX, $pointString, $matches)) {
        throw new \InvalidArgumentException(
            sprintf('Invalid point format. Expected format matching %s, got: %s', self::POINT_REGEX, $pointString)
        );
    }
    
    return new self((float)$matches[1], (float)$matches[2]);
}

Copy link
Owner

Choose a reason for hiding this comment

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

@coderabbitai Perhaps POINT_REGEX can be broken down into reusable components that align with your suggestion for __construct? Suggest a holistic refactoring.

Copy link

Choose a reason for hiding this comment

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

@martin-georgiev Here's a holistic refactoring suggestion that breaks down the POINT_REGEX into reusable components and aligns them with constructor validation:

<?php

declare(strict_types=1);

namespace MartinGeorgiev\Doctrine\DBAL\Types\ValueObject;

/**
 * @since 3.1
 *
 * @author Sébastien Jean <sebastien.jean76@gmail.com>
 */
final class Point implements \Stringable
{
    /**
     * Maximum number of decimal places allowed for coordinate values.
     */
    public const MAX_DECIMAL_PLACES = 6;
    
    /**
     * Regex pattern for a single coordinate with up to MAX_DECIMAL_PLACES decimal places.
     */
    public const COORDINATE_PATTERN = '-?\d+(?:\.\d{1,' . self::MAX_DECIMAL_PLACES . '})?';
    
    /**
     * Regex pattern for validating a PostgreSQL POINT format string.
     * Format: (x,y) where x and y are numbers with up to MAX_DECIMAL_PLACES decimal places.
     */
    public const POINT_REGEX = '/\((' . self::COORDINATE_PATTERN . '),\s*(' . self::COORDINATE_PATTERN . ')\)/';
    
    /**
     * @throws \InvalidArgumentException When coordinates exceed MAX_DECIMAL_PLACES decimal places
     */
    public function __construct(
        private readonly float $x,
        private readonly float $y,
    ) {
        self::validateCoordinate($x, 'x');
        self::validateCoordinate($y, 'y');
    }

    public function __toString(): string
    {
        return \sprintf('(%f, %f)', $this->x, $this->y);
    }

    public function getX(): float
    {
        return $this->x;
    }

    public function getY(): float
    {
        return $this->y;
    }
    
    /**
     * Creates a Point from a string in PostgreSQL POINT format.
     *
     * @param string $pointString String in format (x,y)
     * @return self
     * @throws \InvalidArgumentException When the string doesn't match the expected format
     */
    public static function fromString(string $pointString): self
    {
        if (!preg_match(self::POINT_REGEX, $pointString, $matches)) {
            throw new \InvalidArgumentException(
                sprintf('Invalid point format. Expected format matching %s, got: %s', self::POINT_REGEX, $pointString)
            );
        }
        
        return new self((float)$matches[1], (float)$matches[2]);
    }
    
    /**
     * Validates that a coordinate doesn't exceed MAX_DECIMAL_PLACES decimal places.
     *
     * @param float $value The coordinate value to validate
     * @param string $name The name of the coordinate (for error messages)
     * @throws \InvalidArgumentException When the coordinate exceeds MAX_DECIMAL_PLACES decimal places
     */
    private static function validateCoordinate(float $value, string $name): void
    {
        $stringValue = (string)$value;
        
        if (preg_match('/\.\d{' . (self::MAX_DECIMAL_PLACES + 1) . ',}$/', $stringValue)) {
            throw new \InvalidArgumentException(
                sprintf(
                    '%s coordinate must not exceed %d decimal places. Given: %s',
                    ucfirst($name),
                    self::MAX_DECIMAL_PLACES,
                    $stringValue
                )
            );
        }
    }
}

This refactoring:

  1. Extracts the precision limit to MAX_DECIMAL_PLACES constant
  2. Creates a reusable COORDINATE_PATTERN for single coordinates
  3. Rebuilds POINT_REGEX using these components
  4. Adds constructor validation with the same precision limits
  5. Adds a fromString factory method for string parsing
  6. Centralizes validation logic in a private method

This approach ensures consistency between regex validation and constructor validation while providing a clean interface for creating and validating Point objects.

Copy link
Owner

Choose a reason for hiding this comment

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

@seb-jean, see above, I meant something similar with this comment, but didn't express myself very clearly. Sorry for that.

Copy link

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the changes :).

private readonly float $x,
private readonly float $y,
) {
$this->validateCoordinate($x, 'x');
$this->validateCoordinate($y, 'y');
}

public function __toString(): string
{
return \sprintf('(%f, %f)', $this->x, $this->y);
}

public function getX(): float
{
return $this->x;
}

public function getY(): float
{
return $this->y;
}

public static function fromString(string $pointString): self
{
if (!\preg_match(self::POINT_REGEX, $pointString, $matches)) {
throw new \InvalidArgumentException(
\sprintf('Invalid point format. Expected format matching %s, got: %s', self::POINT_REGEX, $pointString)
);
}

return new self((float) $matches[1], (float) $matches[2]);
}

private function validateCoordinate(float $value, string $name): void
{
$stringValue = (string) $value;

if (!\preg_match('/^-?\d+(\.\d{1,6})?$/', $stringValue)) {
throw new \InvalidArgumentException(
\sprintf('Invalid %s coordinate format: %s', $name, $stringValue)
);
}
}
}
Loading
Loading