idsegments as option #15753
idsegments as option #15753
Conversation
There was a problem hiding this comment.
Thanks @mikkeschiren for creating your first contribution! I left some comments in the review, let me know if you have any questions or need some help.
| if (isset($idsegments)) { | ||
| $segments = array_merge($segments, $idsegments); | ||
| $segments = array_unique($segments); | ||
| } |
There was a problem hiding this comment.
I think the code here can be simplified. $segments = $this->getSegmentsToInvalidateFor($input, $sites); is executed no matter what but can be called twice if --segment is supplied. And the if (isset($idsegments)) { block could be located next to $idsegments w/o the if. Can you make these changes?
| { | ||
| $idSegments = $input->getOption('idsegments'); | ||
| $idSegments = trim($idSegments); | ||
| $idSegments = preg_replace('/\s/', '', $idSegments); |
There was a problem hiding this comment.
trim and preg_replace are redundant here. Only the preg_replace is needed.
| $idSegments = trim($idSegments); | ||
| $idSegments = preg_replace('/\s/', '', $idSegments); | ||
| $idSegments = explode(",", $idSegments); | ||
| $idSegments = array_map('trim', $idSegments); |
There was a problem hiding this comment.
W/ the preg_replace above, this call isn't necessary.
| try { | ||
| $result[] = new Segment($segmentString, $idSites); | ||
| } catch (\Exception $e) { | ||
| return $e->getMessage(); |
There was a problem hiding this comment.
Since this method should return an array of segments, if an error is encountered, can you just output a warning/error and ignore the segment?
|
|
||
| if (empty($idSegments)) { | ||
| return array(null); | ||
| } |
There was a problem hiding this comment.
What's the reason for returning [null]? Should it instead return an empty array so nothing is invalidated? Given it's unexpected we could also output a warning here.
|
@mikkeschiren thanks a lot for this PR. Very appreciated. Do you mind looking at the comments from @diosmosis ? |
|
@mikkeschiren as mentioned in previous comment could you look at the comments from @diosmosis ? |
|
@mikkeschiren I'll close this PR for now as there has been no response to the comments in quite a while but we'll be very happy to do another review should you continue with this PR |
fix #14763