Added support for Italy parcel points #9
Conversation
yekovalenkoa
force-pushed
the
add-italy-country
branch
from
fa80564
to
3c417b1
Compare
yekovalenkoa
force-pushed
the
refactor-php-81
branch
from
2221633
to
5adf2f0
Compare
|
|
||
| class DeliveryItaly implements DeliveryCountryInterface | ||
| { | ||
| private const BASE_URL = 'https://api-it-local-points.easypack24.net/'; |
There was a problem hiding this comment.
Consider saving the link in env and then you don't have to change the bundle every time a new market develops. Am I right that only the base url is different?
There was a problem hiding this comment.
Good idea, let's use configProvider
yekovalenkoa
force-pushed
the
add-italy-country
branch
9 times, most recently
from
cd6baf3
to
744fec6
Compare
phpstan.neon
Outdated
| - | ||
| message: '#.*Extension::processConfiguration.*#' | ||
| path: ./src/DependencyInjection |
There was a problem hiding this comment.
You can add enter to clean the files from the red alert's as here
| ?ClientInterface $client = null, | ||
| ) { | ||
| $this->client = $client ?? new GuzzleClient( | ||
| [ | ||
| 'base_uri' => ConfigProvider::BASE_URL, | ||
| 'base_uri' => $this->configProvider->baseUrl, |
There was a problem hiding this comment.
API version can be included in base url? Since we have base url in env, we can change api version dynamically.
What I mean is removing the API version env variable.
There was a problem hiding this comment.
In my opinion, it is better to keep the version as a variable and not put it in base_url
There was a problem hiding this comment.
Any specific reason? :D
Take a look at the variables in env that end with _URL
src/Command/FindPoints.php
Outdated
| @@ -27,7 +30,7 @@ public function findPoints(FindPointsRequest $request): FindPointsResponse | |||
| { | |||
| $httpRequest = new HttpRequest( | |||
| $request->getMethod(), | |||
| new Uri(ConfigProvider::API_VERSION . $request->getRequestUrl()), | |||
| new Uri($this->configProvider->apiVersion . $request->getRequestUrl()), | |||
There was a problem hiding this comment.
Refers to the comment above ☝️ Then the API version does not need to be appended to every request. Am I right?
| if (!isset($operatingHours['customer'])) { | ||
| return $self; | ||
| } | ||
|
|
||
| if (!is_array($operatingHours['customer'])) { | ||
| return $self; | ||
| } |
There was a problem hiding this comment.
| if (!isset($operatingHours['customer'])) { | |
| return $self; | |
| } | |
| if (!is_array($operatingHours['customer'])) { | |
| return $self; | |
| } | |
| if (!empty($operatingHours['customer'] ?? [])) { | |
| return $self; | |
| } |
#whatever
There was a problem hiding this comment.
In my opinion, the suggested code does not check whether a variable is an array
| } | ||
|
|
||
| foreach ($operatingHours['customer'] as $day => $hours) { | ||
| if (isset($self->{strtolower($day)})) { |
There was a problem hiding this comment.
Please note that if the partner changes the API or naming convention, the data will be invalid or not parsed correctly.
There was a problem hiding this comment.
This is a phenomenon that applies to every integration in the world and to every field in the response API, not just this specific case. For this reason, the partner provides us with a versioned API. So we trust the partner and at the same time we do not display this field if it is empty, verifying carefully a few lines back.
There was a problem hiding this comment.
Ok, but you have hardcoded the names of the days of the week.
Be careful with this because it is slippery: $self->{strtolower($day)}.
yekovalenkoa
force-pushed
the
add-italy-country
branch
4 times, most recently
from
fd66bfb
to
bdb5f58
Compare
yekovalenkoa
force-pushed
the
add-italy-country
branch
from
bdb5f58
to
5ea9532
Compare
No description provided.