review-again and Merge branch 'clear-code-before' into clear-code #1096
Conversation
|
@ruflin, Something wrong again, but with build, because I review code again with diff |
| if (!isset($this->_params[$key])) { | ||
| $this->_params[$key] = array(); | ||
| } | ||
| /** |
There was a problem hiding this comment.
Please remove this note. Lets keep the logic the same in this PR, so I suggest to uncomment the below again (especially as the tests fail).
There was a problem hiding this comment.
@ruflin, I check this in 5.4 http://sandbox.onlinephpfunctions.com/code/8dc8d94b0ccf05c77dffaf2c3b69bda702463f29
error_reporting(E_ALL);
$a = [];
$a['a']['b']['c'][] = 'value';
echo $a === ['a' => ['b' => ['c' => ['value']]]]; // 1
There was a problem hiding this comment.
Its not the change thats the problem, its that there is no need to have it commented out with a TODO comment.
|
In general LGTM. I don't think something is wrong with the build as the error is consistent across all 4 builds: https://travis-ci.org/ruflin/Elastica/jobs/131748215#L4437 Could it be related to the changes you made / commented out? There seems to be a true / false mix somewhere potentially. I didn't check in detail yet. |
| if (!array_key_exists('aggs', $this->_params)) { | ||
| $this->_params['aggs'] = array(); | ||
| } | ||
| /** |
There was a problem hiding this comment.
Another one here.
If this is an issue, aggs should be set in the constructor.
|
Please remove the code rearrangement (exceptions being thrown elsewhere) to a separate PR so it can be discussed independently of the array syntax changes. I'd also like to see the true/false changes as another separate PR since it doesnt relate to the syntax changes either, making it easier to discuss and track. |
|
I tried to investigate further into the composer issue, and I'm not sure what happens. It uses now composer 1.1.1 but the 5.4 build still fails. See https://travis-ci.org/ruflin/Elastica/jobs/133096440#L1777 The strange part is that if I test it locally with PHP 5.6 the --no-dev works as expected, there on travis it seems it doesn't? I need to test it with 5.4 it seems. Any other ideas what the issue is? |
|
Compare with https://travis-ci.org/ruflin/Elastica/jobs/128814644 - see only ...
Problem 1
...
Installation request for aws/aws-sdk-php ~3.0 -> satisfiable by aws/aws-sdk-php
... |
|
@ReenExe Sorry for the late answer. Not sure what you try to say with the above? Any idea how we could fix it? |
|
@ruflin, Sorry my mistake in previous comment. |
|
@ReenExe Yep, it seems like the version didn't change in the PR linked. I'm currently trying to find a work around for this issue (again) :-) |
|
Ok, I think I managed to find a work around for the PHP 5.4 issue. @ReenExe I did also apply linting and interesting it converts all [] into array(...): See #1113 So I assume changing everything to the short syntax would only changing the entry here and apply linting? https://github.com/ruflin/Elastica/blob/master/.php_cs#L19 No manual work needed hopefully :-) |
|
@ruflin, I see, so sorry - but for me to resolve conflict need make new fork - and merge my changes to review it before make new PR - so i do it, and close this to reopen new. |
|
@ReenExe Ok, but try to make it the automated way (no manual work). Please push the array changes separate. That means I can close this PR? |
|
@ruflin, Yes, close. I try find how automate or some like if ($condition) {
throw new \Exception('some');
} else {
return $b
}to if ($condition) {
throw new \Exception('some');
}
return $bBut think - for this automated need contribute to CS |
|
@ReenExe Yeah, for automation I was only referring to the array stuff. |
No description provided.