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 getId/id return type for entities #208

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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
6 changes: 6 additions & 0 deletions spec/Entity/BillingDataSpec.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,12 @@ function let(): void
);
}

function it_has_no_id_by_default(): void
{
$this->getId()->shouldReturn(null);
$this->id()->shouldReturn(null);
}

function it_implements_billing_data_interface(): void
{
$this->shouldImplement(BillingDataInterface::class);
Expand Down
27 changes: 27 additions & 0 deletions spec/Entity/InvoiceSequenceSpec.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
<?php

declare(strict_types=1);

namespace spec\Sylius\InvoicingPlugin\Entity;

use PhpSpec\ObjectBehavior;
use Sylius\InvoicingPlugin\Entity\InvoiceSequenceInterface;

final class InvoiceSequenceSpec extends ObjectBehavior
{
function it_implements_invoice_sequence_interface(): void
{
$this->shouldImplement(InvoiceSequenceInterface::class);
}

function it_has_zero_index_after_initialized(): void
{
$this->getIndex()->shouldReturn(0);
}

function it_increments_index(): void
{
$this->incrementIndex();
$this->getIndex()->shouldReturn(1);
}
}
69 changes: 69 additions & 0 deletions spec/Entity/InvoiceShopBillingDataSpec.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
<?php

declare(strict_types=1);

namespace spec\Sylius\InvoicingPlugin\Entity;

use PhpSpec\ObjectBehavior;
use Sylius\Component\Resource\Model\ResourceInterface;
use Sylius\InvoicingPlugin\Entity\InvoiceShopBillingDataInterface;

final class InvoiceShopBillingDataSpec extends ObjectBehavior
{
function it_implements_tax_item_interface(): void
{
$this->shouldImplement(InvoiceShopBillingDataInterface::class);
}

function it_implements_resource_interface(): void
{
$this->shouldImplement(ResourceInterface::class);
}

function it_has_no_id_by_default(): void
{
$this->getId()->shouldReturn(null);
}

function its_company_is_mutable(): void
{
$this->setCompany('Ragnarok');
$this->getCompany()->shouldReturn('Ragnarok');
}

function its_tax_id_is_mutable(): void
{
$this->setTaxId('1100110011');
$this->getTaxId()->shouldReturn('1100110011');
}

function its_country_code_is_mutable(): void
{
$this->setCountryCode('US');
$this->getCountryCode()->shouldReturn('US');
}

function its_street_is_mutable(): void
{
$this->setStreet('Blue Street');
$this->getStreet()->shouldReturn('Blue Street');
}

function its_city_is_mutable(): void
{
$this->setCity('New York');
$this->getCity()->shouldReturn('New York');
}

function its_postcode_is_mutable(): void
{
$this->setPostcode('94111');
$this->getPostcode()->shouldReturn('94111');
}

function its_representative_is_mutable(): void
{
$this->setRepresentative('Billie Jackson');
$this->getRepresentative()->shouldReturn('Billie Jackson');
}
}
10 changes: 8 additions & 2 deletions spec/Entity/InvoiceSpec.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,13 @@ function let(
InvoiceShopBillingDataInterface $shopBillingData,
OrderInterface $order
): void {
$issuedAt = \DateTimeImmutable::createFromFormat('Y-m', '2019-01');
$order->getNumber()->willReturn('000000001');

$issuedAt = new \DateTimeImmutable('2019-01-23 15:45:30');

$this->beConstructedWith(
'7903c83a-4c5e-4bcf-81d8-9dc304c6a353',
$issuedAt->format('Y/m') . '/000000001',
'2019/01/000000001',
$order,
$issuedAt,
$billingData,
Expand Down Expand Up @@ -72,8 +74,11 @@ function it_has_data(
OrderInterface $order
): void {
$this->id()->shouldReturn('7903c83a-4c5e-4bcf-81d8-9dc304c6a353');
$this->getId()->shouldReturn('7903c83a-4c5e-4bcf-81d8-9dc304c6a353');
$this->number()->shouldReturn('2019/01/000000001');
$this->order()->shouldReturn($order);
$this->orderNumber()->shouldReturn('000000001');
$this->issuedAt()->shouldBeLike(new \DateTimeImmutable('2019-01-23 15:45:30'));
$this->billingData()->shouldReturn($billingData);
$this->currencyCode()->shouldReturn('USD');
$this->localeCode()->shouldReturn('en_US');
Expand All @@ -82,5 +87,6 @@ function it_has_data(
$this->taxItems()->shouldBeLike(new ArrayCollection([$taxItem->getWrappedObject()]));
$this->channel()->shouldReturn($channel);
$this->shopBillingData()->shouldReturn($shopBillingData);
$this->paymentState()->shouldReturn(InvoiceInterface::PAYMENT_STATE_COMPLETED);
}
}
6 changes: 6 additions & 0 deletions spec/Entity/LineItemSpec.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,12 @@ function let(): void
);
}

function it_has_no_id_by_default(): void
{
$this->getId()->shouldReturn(null);
$this->id()->shouldReturn(null);
}

function it_implements_line_item_interface(): void
{
$this->shouldImplement(LineItemInterface::class);
Expand Down
6 changes: 6 additions & 0 deletions spec/Entity/TaxItemSpec.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,12 @@ function it_implements_resource_interface(): void
$this->shouldImplement(ResourceInterface::class);
}

function it_has_no_id_by_default(): void
{
$this->getId()->shouldReturn(null);
$this->id()->shouldReturn(null);
}

function it_has_proper_tax_item_data(): void
{
$this->label()->shouldReturn('VAT (23%)');
Expand Down
6 changes: 3 additions & 3 deletions src/Entity/BillingData.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
/** @final */
class BillingData implements BillingDataInterface, ResourceInterface
{
protected int $id;
protected ?int $id;

protected string $firstName;

Expand Down Expand Up @@ -60,12 +60,12 @@ public function __construct(
$this->company = $company;
}

public function getId(): int
public function getId(): ?int
{
return $this->id();
}

public function id(): int
public function id(): ?int
{
return $this->id;
}
Expand Down
2 changes: 1 addition & 1 deletion src/Entity/BillingDataInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@

interface BillingDataInterface
{
public function id(): int;
public function id(): ?int;

public function firstName(): string;

Expand Down
53 changes: 26 additions & 27 deletions src/Entity/InvoiceShopBillingData.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,7 @@
/** @final */
class InvoiceShopBillingData implements InvoiceShopBillingDataInterface, ResourceInterface
{
/** @var mixed */
protected $id;
protected ?int $id;

protected ?string $company = null;

Expand All @@ -35,7 +34,7 @@ class InvoiceShopBillingData implements InvoiceShopBillingDataInterface, Resourc

protected ?string $representative = null;

public function getId()
public function getId(): ?int
{
return $this->id;
}
Expand All @@ -45,64 +44,64 @@ public function getCompany(): ?string
return $this->company;
}

public function getTaxId(): ?string
public function setCompany(?string $company): void
Copy link
Member Author

Choose a reason for hiding this comment

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

Methods re-arranged in order of class variables declaration/doctrine entities declaration

{
return $this->taxId;
$this->company = $company;
}

public function getCountryCode(): ?string
public function getTaxId(): ?string
{
return $this->countryCode;
return $this->taxId;
}

public function getStreet(): ?string
public function setTaxId(?string $taxId): void
{
return $this->street;
$this->taxId = $taxId;
}

public function getCity(): ?string
public function getCountryCode(): ?string
{
return $this->city;
return $this->countryCode;
}

public function getPostcode(): ?string
public function setCountryCode(?string $countryCode): void
{
return $this->postcode;
$this->countryCode = $countryCode;
}

public function getRepresentative(): ?string
public function getStreet(): ?string
{
return $this->representative;
return $this->street;
}

public function setCompany(?string $company): void
public function setStreet(?string $street): void
{
$this->company = $company;
$this->street = $street;
}

public function setTaxId(?string $taxId): void
public function getCity(): ?string
{
$this->taxId = $taxId;
return $this->city;
}

public function setCountryCode(?string $countryCode): void
public function setCity(?string $city): void
{
$this->countryCode = $countryCode;
$this->city = $city;
}

public function setStreet(?string $street): void
public function getPostcode(): ?string
{
$this->street = $street;
return $this->postcode;
}

public function setCity(?string $city): void
public function setPostcode(?string $postcode): void
{
$this->city = $city;
$this->postcode = $postcode;
}

public function setPostcode(?string $postcode): void
public function getRepresentative(): ?string
{
$this->postcode = $postcode;
return $this->representative;
}

public function setRepresentative(?string $representative): void
Expand Down
26 changes: 13 additions & 13 deletions src/Entity/InvoiceShopBillingDataInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,33 +15,33 @@

interface InvoiceShopBillingDataInterface
{
public function getId();

public function getTaxId(): ?string;
public function getId(): ?int;

public function getCompany(): ?string;

public function getCountryCode(): ?string;

public function getStreet(): ?string;

public function getCity(): ?string;

public function getPostcode(): ?string;

public function getRepresentative(): ?string;

public function setCompany(?string $company): void;

public function getTaxId(): ?string;

public function setTaxId(?string $taxId): void;

public function getCountryCode(): ?string;

public function setCountryCode(?string $countryCode): void;

public function getStreet(): ?string;

public function setStreet(?string $street): void;

public function getCity(): ?string;

public function setCity(?string $city): void;

public function getPostcode(): ?string;

public function setPostcode(?string $postcode): void;

public function getRepresentative(): ?string;

public function setRepresentative(?string $representative): void;
}
7 changes: 3 additions & 4 deletions src/Entity/LineItem.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@
/** @final */
class LineItem implements LineItemInterface, ResourceInterface
{
/** @var mixed */
protected $id;
protected ?int $id;

protected InvoiceInterface $invoice;

Expand Down Expand Up @@ -64,12 +63,12 @@ public function __construct(
$this->taxRate = $taxRate;
}

public function getId()
public function getId(): ?int
Copy link
Member

Choose a reason for hiding this comment

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

I would be for leaving it as it was before, without return type to keep id more customizable and consistent with all Sylius resources. I know that our mapping is configured for using integers, but it is relatively easy to change the strategy to use for example UUIDs instead of integers.

Copy link
Member Author

@diimpp diimpp Dec 3, 2021

Choose a reason for hiding this comment

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

I would agree that getId(): ?int is opinionated, but not because somebody might want to make that UUID, but because sylius resource bundle doesn't define type-hint.

Though this is concrete implementation that you're marking final. In such case it's totally alright to make it int and not to support some doctrine hacks for 3rd party.
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Did you see, that BillingData id is already int in class, but ShopBillingData is not? This is inconsistency and my PR is solving this as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should Invoice's protected string $id became @mixed as well and constructor's string type-hint removed, because somebody might want to change DB id to int?

{
return $this->id();
}

public function id()
public function id(): ?int
{
return $this->id;
}
Expand Down
2 changes: 1 addition & 1 deletion src/Entity/LineItemInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@

interface LineItemInterface
{
public function id();
public function id(): ?int;

public function invoice(): InvoiceInterface;

Expand Down
Loading