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

Housekeeping part two #764

Merged
merged 3 commits into from
Jan 30, 2015
Merged

Conversation

im-denisenko
Copy link
Contributor

This PR is a next step after #751. @XWB forgot about /test/ directory.

I add php-cs-fixer to require-dev section of composer.json, and create a config file for that tool.
Config uses symfony level as default and 3 fixers from contrib.
Now anyone can run ./vendor/bin/php-cs-fixer fix from the root of git repo and get properly defined result.

Also I suggest to enable short_array_syntax fixer from contrib level: short syntax already used in elastica, and now old style and new style mixed together. I did not do this because it's a big change.
If you ok with that, let me know, I will add one more commit, which will convert all arrays to new syntax.

- php-cs-fixer added to require-dev
- config file added
@ruflin
Copy link
Owner

ruflin commented Jan 30, 2015

My question for the old style is: In case we use the old style everywhere, will Elastica still run with PHP 5.3? PHP 5.3 still seems to be the most popular version: http://w3techs.com/technologies/details/pl-php/5/all

ruflin added a commit that referenced this pull request Jan 30, 2015
@ruflin ruflin merged commit 38c441f into ruflin:master Jan 30, 2015
@im-denisenko
Copy link
Contributor Author

It can, but in that case 5.3 must be added back to travis.yml, new arrays must be converted back, and guzzle must be removed from dependencies. And yeah, "We again support 5.3" record must be added to changes.txt :)

PHP 5.3 is a popular version, sure, but it's dead since august of 2014. Users of php 5.3 still can use elastica 1.4.2, which was released just a week ago. Also, more libraries will abandon 5.3 => faster 5.3 will finally die.

@im-denisenko im-denisenko deleted the housekeeping-part-two branch January 30, 2015 06:25
@merk
Copy link
Collaborator

merk commented Jan 30, 2015

I am against bumping the required PHP version just to use short array syntax. If thats the only reason for the bump there is no point.

If bumping to access other features to have tidier and better maintained code then it makes sense to do so.

@ruflin
Copy link
Owner

ruflin commented Jan 31, 2015

What I suggest is that we add PHP 5.3 to the build, but don't mark it as required (as we did before). I would not change the communication about the compatibility. It is still 5.4+, but it can work with 5.3. So as long as it is not an extra effort, we keep it compatible with PHP 5.3, but it can be that some of the features don't work.

@merk @im-denisenko What do you think?

@merk
Copy link
Collaborator

merk commented Jan 31, 2015

If short array syntax is used in the codebase there is no point.

If you're bumping required versions why not bump it to 5.5? Though I'd prefer to see the library bump to v2 and start following semver than dropping PHP support.

@im-denisenko
Copy link
Contributor Author

I think we must either provide full support for 5.3 or not support 5.3 at all, because only real difference between php versions for elastica right now is a new syntax sugar which will throw fatal errors in 5.3.

// cc @jdeniau who "take the responsability of removing the 5.3 support" in #738(comment)

@jdeniau
Copy link
Contributor

jdeniau commented Jan 31, 2015

I agree totally with @im-denisenko , event debian packages in 5.4 now, but understand the point of @ruflin
As said it is quite easy to restore 5.3 support. I think there is only one reference to short array syntax in the code.

If everyone is OK for re-adding 5.3 support I'll make a PR next week.

@ruflin
Copy link
Owner

ruflin commented Feb 1, 2015

@jdeniau I suggest we try to readd it by removing the array syntax. Perhaps we realise then that with the recent refactoring also some other new "things" got introduced. Would be nice to discuss it directly with a pull request to see the potential issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants