Plugin on the SyliusStore

[Zales0123]

Hello! I come to you as a Plugin Curator at Sylius to share some feedback regarding this one :) First of all, thank you for your amazing work and fast response for the worldwide crisis - it shows the power of and open-source community 💪

The biggest concern

The thing that troubles me the most is the requirement of PHP 7.4. Although I'm 100% for choosing the newest technology as possible 👌, I wonder how many Sylius users would be able to take benefit of this plugin with the requirements for Sylius ^1.7 and PHP 7.4 🤔. It does not block the addition of this plugin to the Sylius Store, of course, but it would be good if you could rethink the 7.4-only approach - or answer me with some reasons for such a choice 🖖 :) The other thing is, we don't have the official support for PHP 7.4 in Sylius yet, but it should not be a problem (it works well).

Lesser issues

1. In the installation, instruction interfaces are missing for entities (ClickNCollectShippingMethodInterface for ShippingMethod and ClickNCollectShipmentInterface for Shipment)
2. I had a strange issue, I've ordered with collect time at 3:00-3:20pm

But on the order it was 2 hours before 😄

If I'm missing something (a configuration or a point of such a feature), I would be happy to hear (read) it 🚀

3. I had also some problems with showing a proper collect time in "Click 'N' Collect" section for my custom destination... It's strange, as it worked well for Lille and Paris

I would love to see your point on the topics I've mentioned. One more time, thank you for such an awesome contribution to our ecosystem 🖖

[jacquesbh]

We've done the Sylius 1.6 compatibility (and PHP ~7.2), if you want 💪.

https://github.com/coopTilleuls/CoopTilleulsSyliusClickNCollectPlugin/compare/master...monsieurbiz:sylius-1.6?expand=1

We've also updated the recipe to be more precise and the tests CI is totally taking care of that. We haven't updated the README yet.

The hour issue is just about Timezones I think. We've had a lot of issues with FullCalendar in a project and it's really a problem. Especially since Sylius admin panel is UTC (if server is in UTC).

[dunglas]

Hi @Zales0123, and thanks for your review!

Regarding PHP 7.4 and Sylius 1.7, when we start a new project we always use the latest versions of the dependencies to reduce the maintenance burden and increase the code quality. As you can see in @jacquesbh's proposal, not using the latest features makes the code less reliable, less readable and harder to maintain. We prefer pushing users to upgrade to use a new project like this one than supporting old versions of PHP (and other dependencies).

1. I'm not sure of what you mean, the interfaces are mentioned section 6 ("update the native entities")
2. It looks like a timezone issue. It should be handled properly here https://github.com/coopTilleuls/CoopTilleulsSyliusClickNCollectPlugin/blob/master/src/Form/Extension/ShipmentTypeExtension.php#L72 (at least it works with my config), but it looks like there still is a problem. Can you provide me your browser's timezone, and the timezone configured in php.ini? Any help to debug this would be very welcome. Because of browsers limitations, the hour should be using GMT client-side, and converted to the good timezone server-side (by the snippet I linked to).
3. Can you give a bit more information about this problem?

Best regards!

[Zales0123]

Hello @dunglas, thank you for your answer!

1. I had a moment of blindness, definitely, these interfaces are there 😄
2. Definitely a timezone issue 👍 I think it was also a problem with my configuration (I've switched to the PHP7.4 which apparently had no timezone configured 😱), it works well right now. Maybe it could be highlighted in the installation instruction that some problems with timezones may occur in case of misconfiguration? No hard opinion in that topic, generally 👌
3. I had a problem when using a custom location for Click'n'Collect (other than Paris or Lille) - it was not displayed as a reserved time block. I've just checked it out and it seems to work - so either my laptop had a worse day, or me 💃

I totally agree with the approach to use the newest possible versions of dependencies 👌 My only concern was how many people would be able to use this (with no doubts very useful) plugin, as, from my experience, it's often hard for people to switch to newer PHP versions, especially with the application that is already on the production. Again, I see no blocker in that, it's just thinking out loud for the possible issues.

Maybe it would be good to link the compatibility changes provided by @jacquesbh somewhere in the README? Something like "this plugin is based on PHP 7.4 and Sylius 1.7, if you want to use it with 7.2/1.6 check out this changes"? It would not force you to support dependencies you don't want to and still give an alternative for people that want/need to do that 🖖

Anyway - it's a good piece of code in the good moment of time. I would be happy to place it in our Sylius Store. I will probably do it today and ping you with a link :)

Thanks! 🚀

[dunglas]

Regarding 2, actually it can affect all Sylius installations, regardless of if this plugin is used or not. Couldn't you provide an option to set the timezone for a given channel? 👍 to add a link in the readme to @jacquesbh's version!