Skip to content

Add Size::dimensions() for destructuring (fixes #1480)#1485

Closed
Abdooo2235 wants to merge 5 commits into
Intervention:developfrom
Abdooo2235:fix/1480-size-dimensions
Closed

Add Size::dimensions() for destructuring (fixes #1480)#1485
Abdooo2235 wants to merge 5 commits into
Intervention:developfrom
Abdooo2235:fix/1480-size-dimensions

Conversation

@Abdooo2235
Copy link
Copy Markdown

Summary

Closes #1480.

Adds Size::dimensions(): array{0: int, 1: int} so consumers can destructure
size into a [width, height] int pair:

[$width, $height] = $image->size()->dimensions();

Why not change [$w, $h] = $image->size() directly?

The literal syntax in the original issue would require changing
ArrayAccess::offsetGet on Size. That offset is currently the way
Polygon (Size's parent) exposes the four corner Point objects, and
Size::setWidth()/setHeight() rely on it internally
(\$this[1]->setX(...), etc.). Repurposing those offsets to return ints
would be a breaking change for every caller and for the class's own setters.

dimensions() is the non-breaking alternative: same ergonomics for
destructuring, zero risk for existing code.

Tests

Added to tests/Unit/SizeTest.php:

  • testDimensionsReturnsWidthAndHeightAsInts
  • testDimensionsEnablesArrayDestructuring
  • testDimensionsReflectsSetWidthAndSetHeight
  • testArrayAccessStillReturnsCornerPoints (explicit BC guard)

vendor/bin/phpunit tests/Unit/SizeTest.php passes locally (67 tests, 152 assertions).

Notes

  • SizeInterface updated to include the new method.
  • No changes to Polygon or ArrayAccess semantics.

@olivervogel
Copy link
Copy Markdown
Member

olivervogel commented May 11, 2026

Thank you for the contribution and for taking the time to work on this.

Unfortunately, I can't merge the PR in its current form because the interface change introduce a breaking change.

I'd also prefer the method to be named Size::toArray(), as that would be more consistent with Polygon::toArray(). The current naming feels a bit inconsistent with the existing API conventions.

That said, there is also a larger issue with toArray() itself. Since the method is inherited from Polygon::class and currently used in src/Drivers/Gd/Modifiers/RotateModifier.php as a size to output individual points, so changing its behavior would have unintended side effects.

My actual goal is that one shouldn't have to call a method, but be able to destructure the object directly.

Thanks again.

@Abdooo2235
Copy link
Copy Markdown
Author

Thanks for the detailed feedback — that helps a lot.

Reading your three points, the architectural root seems to be that Size extends Polygon: that's what forces corner-Point semantics through ArrayAccess and blocks direct [$w, $h] = $image->size() destructuring, and it's also why toArray() can't be repurposed without breaking RotateModifier.

Would you accept a refactor where Size is decoupled from Polygon and implements ArrayAccess<int, int> directly — so $size[0] is width and $size[1] is height? Existing corner-Point access would move to an explicit method (e.g. corners(): array<PointInterface>), and the few internal callers (RotateModifier::apply(), Size::setWidth/setHeight, etc.) would be updated to use it. That gives you the destructuring API you described, keeps Polygon::toArray() untouched, and contains the BC impact to a clearly documented surface.

I'm happy to open a new PR with that scope if you'd consider merging it. Otherwise I'll close this out and wait for a future major release.

@olivervogel
Copy link
Copy Markdown
Member

olivervogel commented May 12, 2026

I think decoupling Size from Polygon would be the best way to solve this.

I have already tried that. However, I invested rather little time and noticed that it is more complex than can be done just like that.

I believe one issue actually is this area: https://github.com/Intervention/image/blob/develop/src/Drivers/Gd/Modifiers/RotateModifier.php#L88-L94

$cutout = (new Size(
    imagesx($frame->native()),
    imagesy($frame->native()),
    $container->pivot()
))->alignHorizontally(Alignment::CENTER)
    ->alignVertically(Alignment::CENTER)
    ->rotate($this->rotationAngle());

A rectangular area is created from Size, but then methods are used that belong to Polygon. The blocker for simple decoupling is that size musts implement rotate() and the other methods even though they are not part of SizeInterface.

I think it makes sense to clean this up in the process. Maybe something like this would work:

$cutout = Polygon::fromSize(new Size(
    imagesx($frame->native()),
    imagesy($frame->native()),
    $container->pivot()
))->alignHorizontally(Alignment::CENTER)
  ->alignVertically(Alignment::CENTER)
  ->rotate($this->rotationAngle());

This way the decoupled Size must not implement rotate() can be consumed by imagefilledpolygon() in https://github.com/Intervention/image/blob/develop/src/Drivers/Gd/Modifiers/RotateModifier.php#L104

I'm not sure atm whether that's the only part that's problematic.

I also noticed that Size is linked with Intervention\Image\Geometry\Rectangle. Unfortunately this whole arrangement is rather a design flaw and not ideal. I have a feeling this can not be cleaned up without breaking changes.

@Abdooo2235
Copy link
Copy Markdown
Author

Appreciate the detailed pointer — the RotateModifier chain was exactly the kind of hidden coupling I would have missed on a first pass.

Polygon::fromSize() as a factory makes sense; it keeps Size minimal and pushes the polygon behaviour to where it's actually needed. I'll also audit Rectangle and any other call sites that lean on the inheritance before settling on the final design.

I'll put together a draft PR with the decoupled Size, the factory, and the migrated call sites, and include a clear list of the BC breaks in the description so you can judge the impact. Going for a clean cut over a transition layer unless you'd prefer otherwise — easier to reason about, and you've already flagged that a major release seems appropriate.

I'll open the new PR (rather than reopen this one) so the conversation stays focused. No timeline promises, but I'll keep it scoped.

@olivervogel
Copy link
Copy Markdown
Member

I had some time and sketched something out: #1487

@Abdooo2235
Copy link
Copy Markdown
Author

Beautiful — you built exactly what we were converging on, including the keyed-destructuring (['width' => $w]) variant which I hadn't proposed. I'll close this out and move my own conversation to #1487. Thanks for the speed.

@Abdooo2235 Abdooo2235 deleted the fix/1480-size-dimensions branch May 16, 2026 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enable array destructuring of Size::class with width & height

2 participants