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

Implement Countable on Objects in PHP7.2 - Backported from #1378 #1510

Merged
merged 1 commit into from
Jul 2, 2018
Merged

Implement Countable on Objects in PHP7.2 - Backported from #1378 #1510

merged 1 commit into from
Jul 2, 2018

Conversation

jmsche
Copy link

@jmsche jmsche commented Jun 29, 2018

Hi,

As I'm currently stuck with ElasticSearch v1.7, I'd like to submit a PR to backport changes brought by initial PR #1378: Implement Countable on Objects in PHP7.2.

Thanks,

jmsche

@ruflin ruflin merged commit 51faf4f into ruflin:2.x Jul 2, 2018
@ruflin
Copy link
Owner

ruflin commented Jul 2, 2018

@jmsche Thanks for the contribution. Haven't touched the 2.x branch in ages but happy to hear it still works :-)

@jmsche
Copy link
Author

jmsche commented Jul 2, 2018

Thanks for accepting the PR :)

Will you create a new tag/release, or should I target the 2.x branch?

@ruflin
Copy link
Owner

ruflin commented Jul 2, 2018

Please use the 2.x branch. If somehow possible I would not like to do an other 2.x release.

@jmsche
Copy link
Author

jmsche commented Jul 2, 2018

I'm wondering if you can still create a release: the package I manage was updated to use "dev-2.x" of ruflin/elastica, but given this I'd have to update all projects to require as well "ruflin/elastica":"@dev", even if "dev-2.x" is mentioned in my package...

Thanks for considering my request.

@ruflin
Copy link
Owner

ruflin commented Jul 3, 2018

If I do a new release, you would also have to update all packages? Or what is your line looking like at the moment?

The reason I'm very hesitant to do a release because I worry I might break things that work at the moment. It's a long time ago since I touched the 2.x code base so it's hard to think through the problems that could happen.

@jmsche
Copy link
Author

jmsche commented Jul 3, 2018

If you create a new release, I only have to revert the package I manage to use ^2.3 instead of dev-2.x, which allows me not to require "ruflin/elastica":"@dev" in all projects.

I understand why you hesitate to create a new release. But, if I'm not mistaken, only two new commits would be released - mine, and d72afd0 - see 2.3.1...2.x

From what I can see, my changes are ok, and after checking the other commit, they also seem legitimate. Both are backports and should be ok no matter the PHP version. The only problem being, I can't manage to run tests on my computer :/

@ruflin
Copy link
Owner

ruflin commented Jul 3, 2018

The commit I'm much more worried is actually not yours but the other one as it adds error handling for I think Elasticsearch 5.x and I'm not sure about the side effects to 1.x installations here. I would appreciate if for now we could leave it as unreleased and reconsider if we get many request for it.

Please also keep in mind that 1.7 was EOL beginning of last year: https://www.elastic.co/support/eol

@bdlabs
Copy link

bdlabs commented Dec 14, 2018

@jmsche Thanks for the contribution. Haven't touched the 2.x branch in ages but happy to hear it still works :-)

Sorry but not work this branch must be compatible with PHP 5.3.3 and now is it not.
look this #1559

@ruflin
Copy link
Owner

ruflin commented Dec 14, 2018

@jmsche Could you comment on this one. That is what I was worried about doing a release :-( @bdlabs What you can always do is going back one bugfix release to the one you had before.

@bdlabs
Copy link

bdlabs commented Dec 26, 2018

@jmsche Could you comment on this one. That is what I was worried about doing a release :-( @bdlabs What you can always do is going back one bugfix release to the one you had before.

@ruflin I know :) that I did but it's not the right solution.

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.

3 participants