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

Use PHP 7.4 syntax in Sylius components #13012

Merged
merged 2 commits into from
Sep 3, 2021

Conversation

Zales0123
Copy link
Member

Q A
Branch? 1.10
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Related tickets related to #12891
License MIT

I know, it's probably unreviewable (but bundles will be worse 😄). In tests we trust 🖖 🚀

@Zales0123 Zales0123 added the Maintenance CI configurations, READMEs, releases, etc. label Aug 26, 2021
@Zales0123 Zales0123 requested a review from a team as a code owner August 26, 2021 23:35
@Zales0123 Zales0123 mentioned this pull request Aug 26, 2021
10 tasks

/** @var string */
protected $type = TextAttributeType::TYPE;
protected ?string $type = TextAttributeType::TYPE;

/** @var array */
protected $configuration = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
protected $configuration = [];
protected array $configuration = [];


/** @var string */
protected $type = TextAttributeType::TYPE;
protected ?string $type = TextAttributeType::TYPE;

/** @var array */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** @var array */


/** @var RequestStack */
private $requestStack;
private RequestStack $requestStack;

/** @var \SplObjectStorage<Request, ChannelInterface> */
private $requestToChannelMap;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private $requestToChannelMap;
private \SplObjectStorage $requestToChannelMap;

Copy link
Contributor

Choose a reason for hiding this comment

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

There's also private \SplObjectStorage $requestToExceptionMap; below, but can't suggest a change there...


/** @var ChannelInterface */
private $channel;
private ?ChannelInterface $channel;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private ?ChannelInterface $channel;
private ChannelInterface $channel;


/** @var OrderInterface|null */
private $cart;
private ?\Sylius\Component\Core\Model\OrderInterface $cart = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private ?\Sylius\Component\Core\Model\OrderInterface $cart = null;
private ?OrderInterface $cart = null;

@@ -22,8 +22,7 @@
*/
final class SalesDataProvider implements SalesDataProviderInterface
{
/** @var EntityRepository */
private $orderRepository;
private EntityRepository $orderRepository;
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated, but why an interface is not used here and in the constructor?

@@ -15,20 +15,15 @@

class ChannelPricing implements ChannelPricingInterface
{
/** @var int|null */
protected $id;
protected ?int $id = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
protected ?int $id = null;
protected $id;

Copy link
Contributor

Choose a reason for hiding this comment

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

Revert and change /** @var int|null */ to /** @var mixed */


/**
* @var Collection|AddressInterface[]
*
* @psalm-var Collection<array-key, AddressInterface>
*/
protected $addresses;
protected Collection $addresses;

/** @var ShopUserInterface|null */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** @var ShopUserInterface|null */


/**
* @var Collection|AddressInterface[]
*
* @psalm-var Collection<array-key, AddressInterface>
*/
protected $addresses;
protected Collection $addresses;

/** @var ShopUserInterface|null */
protected $user;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
protected $user;
protected ?ShopUserInterface $user = null;

@@ -15,24 +15,17 @@

abstract class Image implements ImageInterface
{
/** @var mixed */
protected $id;
/** @var mixed|null */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** @var mixed|null */
/** @var mixed*/

/** @var mixed */
protected $id;
/** @var mixed|null */
protected $id = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is null here necessary if there's no type-hint?

@@ -35,56 +35,47 @@ class Order extends BaseOrder implements OrderInterface
/** @var ChannelInterface|null */
protected $channel;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
protected $channel;
protected ?ChannelInterface $channel = null;

Copy link
Contributor

Choose a reason for hiding this comment

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

It also missed the $customer above.
Looks like it ignores the cases when the class/interface is not explicitly imported because it's in the same namespace.


/** @var string|null */
protected $localeCode;
protected ?string $localeCode = null;

/** @var BaseCouponInterface|null */
protected $promotionCoupon;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
protected $promotionCoupon;
protected ?BaseCouponInterface $promotionCoupon = null;


/** @var string|null */
protected $localeCode;
protected ?string $localeCode = null;

/** @var BaseCouponInterface|null */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** @var BaseCouponInterface|null */


/** @var BaseTaxonInterface */
protected $mainTaxon;
protected ?\Sylius\Component\Core\Model\TaxonInterface $mainTaxon = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
protected ?\Sylius\Component\Core\Model\TaxonInterface $mainTaxon = null;
protected ?TaxonInterface $mainTaxon = null;

@vvasiloi
Copy link
Contributor

@Zales0123 is it useful or shall I stop? 😄
For some reason I chose to add single comments instead of starting a review, so you've probably received a ton of notifications... sorry.

Comment on lines -20 to +26
/** @var string */
protected $firstName;
protected ?string $firstName = null;

/** @var string */
protected $lastName;
protected ?string $lastName = null;

/** @var string */
protected $localeCode;
protected ?string $localeCode = null;

/** @var ImageInterface */
protected $avatar;
protected ?ImageInterface $avatar = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

There was no null before (in annotation) but now you are adding it. May I ask why?

Copy link
Contributor

Choose a reason for hiding this comment

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

There was no null before (in annotation) but now you are adding it. May I ask why?

@arti0090 The annotation was wrong. Both the getter and the setter allow null values, it's not initialized in the constructor and there's no default value, therefore it can be null in a lot of scenarios.

Copy link
Member

Choose a reason for hiding this comment

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

The truth is, these fields were nullable due to setters definition


/**
* @var Collection|ReviewInterface[]
*
* @psalm-var Collection<array-key, ReviewInterface>
*/
protected $reviews;
protected Collection $reviews;

/** @var float */
protected $averageRating = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
protected $averageRating = 0;
protected float $averageRating = 0;

Copy link
Contributor

Choose a reason for hiding this comment

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

The setter doesn't allow null values, but the getter has ?float as return type, so I'm not sure if it should be nullable.


/** @var int */
protected $onHold = 0;
protected ?int $onHold = 0;

/** @var int */
protected $onHand = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
protected $onHand = 0;
protected ?int $onHand = 0;


/** @var int */
protected $onHold = 0;
protected ?int $onHold = 0;

/** @var int */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** @var int */


/** @var Collection */
protected $channelPricings;
protected Collection $channelPricings;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably have a @psalm-var Collection<array-key, ChannelPricingInterface> annotation

@@ -15,26 +15,19 @@

class ShopBillingData implements ShopBillingDataInterface
{
/** @var int|null */
protected $id;
protected ?int $id = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert and change annotation to mixed.


/** @var string */
protected $calculator;
protected ?string $calculator = null;

/** @var array */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** @var array */


/** @var string */
protected $calculator;
protected ?string $calculator = null;

/** @var array */
protected $configuration = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
protected $configuration = [];
protected array $configuration = [];

@@ -21,14 +21,12 @@ class ShippingMethodRule implements ShippingMethodRuleInterface
/** @var mixed */
protected $id;

/** @var string */
protected $type;
protected ?string $type = null;

/** @var array */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** @var array */

@@ -21,14 +21,12 @@ class ShippingMethodRule implements ShippingMethodRuleInterface
/** @var mixed */
protected $id;

/** @var string */
protected $type;
protected ?string $type = null;

/** @var array */
protected $configuration = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
protected $configuration = [];
protected array $configuration = [];


/** @var \DateTimeInterface|null */
protected $credentialsExpireAt;
protected ?\DateTimeInterface $credentialsExpireAt = null;

/**
* We need at least one role to be able to authenticate
Copy link
Contributor

Choose a reason for hiding this comment

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

Missed this one.

*/
protected $code;
protected ?string $code = null;

/**
* @var Collection|ProvinceInterface[]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @var Collection|ProvinceInterface[]

Comment on lines -20 to +26
/** @var string */
protected $firstName;
protected ?string $firstName = null;

/** @var string */
protected $lastName;
protected ?string $lastName = null;

/** @var string */
protected $localeCode;
protected ?string $localeCode = null;

/** @var ImageInterface */
protected $avatar;
protected ?ImageInterface $avatar = null;
Copy link
Member

Choose a reason for hiding this comment

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

The truth is, these fields were nullable due to setters definition


/**
* @var Collection|ReviewInterface[]
*
* @psalm-var Collection<array-key, ReviewInterface>
*/
protected $reviews;
protected Collection $reviews;

/** @var float */
protected $averageRating = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
protected $averageRating = 0;
protected float $averageRating = 0;


/**
* @var Collection|ReviewInterface[]
*
* @psalm-var Collection<array-key, ReviewInterface>
*/
protected $reviews;
protected Collection $reviews;

/** @var float */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/** @var float */

Comment on lines 29 to 30
/** @var int */
protected $onHand = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/** @var int */
protected $onHand = 0;
protected ?int $onHand = 0;

Comment on lines 30 to 34
/**
* @var Collection|ChannelInterface[]
*
* @psalm-var Collection<array-key, ChannelInterface>
*/
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/**
* @var Collection|ChannelInterface[]
*
* @psalm-var Collection<array-key, ChannelInterface>
*/
/** @psalm-var Collection<array-key, ChannelInterface> */

Comment on lines 23 to 24
/** @var array */
protected $configuration = [];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/** @var array */
protected $configuration = [];
protected array $configuration = [];

Comment on lines 32 to 33
* @var Collection|ShipmentUnitInterface[]
*
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @var Collection|ShipmentUnitInterface[]
*

Comment on lines 45 to 46
/** @var array */
protected $configuration = [];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/** @var array */
protected $configuration = [];
protected array $configuration = [];

Comment on lines 34 to 35
* @var Collection|TaxRateInterface[]
*
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @var Collection|TaxRateInterface[]
*

Comment on lines 40 to 41
* @var Collection|TaxonInterface[]
*
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @var Collection|TaxonInterface[]
*

@Zales0123
Copy link
Member Author

Thank you @vvasiloi for the review! I would never believe someone would be determined enough to take a look at it. I'm impressed 👏 I've fixed (hopefully) most of the comments, I've just left those about collection type hints, as I'm not 100% sure how the IDE would handle these types without them 🤷 . Still, can be improved later 🖖

Thank you! 🎉

@GSadee GSadee merged commit e4b7c8a into Sylius:1.10 Sep 3, 2021
@GSadee
Copy link
Member

GSadee commented Sep 3, 2021

Thanks, Mateusz! 🥇

@Zales0123 Zales0123 deleted the php7.4-syntax-components branch September 3, 2021 13:35
@Zales0123 Zales0123 restored the php7.4-syntax-components branch September 30, 2021 13:26
@Zales0123 Zales0123 deleted the php7.4-syntax-components branch September 30, 2021 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance CI configurations, READMEs, releases, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants