Elasticsearch-php: Compatibility issues with version 7.4.0

[deguif]

Summary of problem or feature request

Since version 7.4.0 was published, it seems some classes were renamed which break compatiblity.

For example, Elasticsearch\Endpoints\Ingest\Pipeline\Delete was renamed to Elasticsearch\Endpoints\Ingest\DeletePipeline

See https://github.com/ruflin/Elastica/issues/1710 for reference.

[ezimuel]

@deguif I know this issue are related to the new code generation that is 100% align with the REST API specification of Elasticsearch (using the script util/GenerateEndpoints.php). Before, everything was managed manually and there were some inconsistencies in the namespace and in the code implementation.

Regarding your example, the API is ingest.delete_pipeline.json that is translated in Ingest namespace with class name DeletePipeline (the _ is translated into camelCase). Previously, the namespace and class name was managed manually.

Unfortunately, I cannot do nothing here apart from helping other projects like Elastica to fix the issues.

[deguif]

Thanks @ezimuel for your answer.

What about adding class aliases to keep the backward compatibility until a new major release is published? Do you think it should be doable on this project?

That can also be done on Elastica, but I think it would be better if it's done here as it would avoid breaking compatibility.

[ezimuel]

@deguif this is a potential fix, let me double check the code first. I'll update you soon. Thanks for the suggestion btw!

[thePanz]

Another solution would be to move those changes into a Major release 8.x, as the class-name changes are not respecting the BC promise in the 7.x branch.

[ezimuel]

@thePanz I cannot continue to maintain such amount of code manually. I need to be consistent with the Elasticsearch API specification, that's why I wrote the code generation tool.
I cannot wait for 8.x release. I'll fix the issue with a new patch release 7.4.1 soon.

[ezimuel]

Btw, this BC break is only for library/projects that use directly (some) endpoint classes. From a user land perspective, the endpoints are still the same. For instance the ingest.delete_pipeline mentioned before has the same function name:

$params = [ /* ... */ ];
$result = $client->ingest()->deletePipeline($params); // as before 7.3

[JamesFreeman]

Just ran into this issue with scout-elasticsearch-driver repo.

I've had to pinpoint my release to 7.3.* for now as it was causing fatal errors in production. It's really not ideal to release a BC on a second point release.

[ezimuel]

@deguif, @thePanz I just sent the PR #968 to fix the issue with the old namespaces. Please review, thanks!

@iamthefreeman I know and I'm sorry for that. This BC breaks is related with some old namespace used in the code. There was some inconsistency with the previous endpoints. Now the code is 100% generated from the REST API specification of Elasticsearch, so no more dead code in the future.

Sorry again, I'll release 7.4.1 as soon #968 will be merged.

[iget-master]

I really disagree with breaking changes on a minor release. Everyone relies that minor releases doesn't contains any intentional breaking changes. The 7.4.0 contains two intentional breaking changes (I say that's intentional because are written on changelog).

This release broke this widely used package https://github.com/babenkoivan/scout-elasticsearch-driver/issues/297 and many of our applications.

I understand that you need to be consistent with elasticsearch API, but I'm pretty sure they doesn't add any breaking changes on minor releases.

[ezimuel]

@iget-master let me try to explain what happened with 7.4.0 release. First of all, I agree with you but there were some difficult condition that drove me in this direction.

The existing code base of elasticsearch-php is updated manually, that means on every release of Elasticsearch we need to go throught all the API specification (reported here) and update the code base and the documentation.

This process requires a lot of time and it's not error free, in fact we had some inconsistencies in the endpoints like missing endpoints, parameters or not updated one.

In order to reduce these inconsistencies and release a client library that is 100% aligned with the API of Elasticsearch, I built a tool in tool/GenerateEndpoints.php. This tool is able to read the API specification and generate the classes for all the Endpoints and Namespaces.

I used this tool to generate the endpoints shipped in 7.4.0 and there was some unexpected BC breaks with previous class naming, because these old names were not following the API specification. For instance, the mtermvectors endpoint was translated manually into MTermVectors class name. The code generation tool provides only a ucword() of the endpoint name and the result is Mtermvectors.
I fixed these BC break naming in #968 and I'm going to release it in 7.4.1 as soon I'll receive feedback on it. I'm very sorry for this BC break but this was unexpected.

Regarding the intentional BC break in 7.4.0 that I wrote it in the release note, I know that we don't have to break stuff in a minor release, this is the idea of semantic version but sometime we need to make (questionable) decision. I decided to introduce two potential BC break because I saw in it a benefit instead of issue.
The fist benefit is about deprecation. I added a trigger for deprecated parameters (according to the API specification) using a E_USER_DEPRECATED . If you use a parameter that is considered deprecated by Elasticsearch the code will generate this warning. Technically speaking, this cannot be considered a BC break because the code will continue to work as before. You will only be informed, in your logging system, about the deprecation. I think this is actually very important because give you a vision about future change, giving you the possibility to update your code without rush.
The second BC break is really minor: when using delete endpoint with an empty id a Missing404Exception exception is thrown. Previously it was an InvalidArgumentException. I introduced this change because it's consistent with the API specification and I don't want to continue to maintain such inconsistency.

Again, I'm really sorry for these changes but I hope you can now understand better the motivation behind this decision.

Just to recap, I'm working on 7.4.1 and release it soon after I'll receive some feedback. This patch release will fix all the unexpected BC breaks, the class naming introduced using the code generation tool. The two (intentional) potential BC breaks reported in the release note of 7.4.0 will remain as is. Thanks!

[ezimuel]

@deguif can you test this PR with Elastica to see if this solves all the BC break issues? Thanks!

[ezimuel]

@iamthefreeman can you test this PR against your scout-elasticsearch-driver project to see if this solves all the BC break issues? Thanks!

[deguif]

@ezimuel Thanks for your work on this issue.

A lot of errors are now fixed, with your PR about the class aliases.
It just remains 2 class not found errors in the ruflin/elastica test suite:

• Error: Class 'Elasticsearch\Endpoints\Cluster\Nodes\Info' not found
• Error: Class 'Elasticsearch\Endpoints\Cluster\Nodes\Stats' not found

[ezimuel]

@deguif thanks for reporting this, I'm looking into it.

[deguif]

Here my composer setup for testing your PR (that might help other people):

{
    "config": {
        "github-oauth": {
            "github.com": "<TOKEN>"
        }
    },
    "require": {
        "elasticsearch/elasticsearch": "dev-fix/967"
    },
    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/ezimuel/elasticsearch-php"
        }
    ]
}

[AegirLeet]

The fist benefit is about deprecation. I added a trigger for deprecated parameters (according to the API specification) using a E_USER_DEPRECATED . If you use a parameter that is considered deprecated by Elasticsearch the code will generate this warning. Technically speaking, this cannot be considered a BC break because the code will continue to work as before. You will only be informed, in your logging system, about the deprecation.

Modern PHP frameworks (Laravel, Symfony) will generally convert every error (even E_USER_DEPRECATED) into an ErrorException and throw that. Unless specifically caught, this will stop execution. Here's Laravel's implementation, for example.

So adding any kind of trigger_error() in a minor release, which should be backwards compatible according to SemVer, is a bad idea.

[ezimuel]

@AegirLeet in a production environment, if you have ini_set('display_errors', 0);, as required, all the PHP error/warning/notice, like PHP Deprecated in our case, are stored in a error log file. So there's no BC break, the code will continue to be executed as expected.

If I convert the E_USER_DEPRECATED in an Exception, I'll break the code for sure and people will require to catch it manually changing the code.

[AegirLeet]

Setting display_errors to off is normal, yes.

But modern PHP code will catch every single error, warning or notice and convert it to an ErrorException. Code is generally expected to never produce an error/warning/notice during normal operation. Any kind of error/warning/notice will log an exception, display an error page to the user (if it's a web request) and halt execution.

So triggering an E_USER_DEPRECATED will have exactly the same effect as throwing an Exception. Both are bad ideas for a minor release.

It's no big deal, i quickly downgraded to 7.3 when the error appeared, but you should probably keep this in mind for future releases.

[ezimuel]

@AegirLeet I see what you mean, but still I think this a problem of how you configure your PHP application in a production environment. Even in Laravel of Symfony you have different environment configuration for logging the PHP events (error/warning/notice/deprecated, etc) as exceptions.

In your opinion, If I cannot use E_USER_DEPRECATED events how I can inform people about deprecation? Just using a @deprecated tag in the phpdoc?

Thanks for your feedback!

[thePanz]

@ezimuel @AegirLeet in Symfony the trigger_error() is widely used to mark deprecated methods and calls.
The following is an example: @trigger_error('Support for mapping keys in multi-line blocks is deprecated since Symfony 4.3 and will throw a ParseException in 5.0.', E_USER_DEPRECATED);

Found in the Yaml parser of Symfony.

[AegirLeet]

In your opinion, If I cannot use E_USER_DEPRECATED events how I can inform people about deprecation? Just using a @deprecated tag in the phpdoc?

I'd use @deprecated tags where applicable (you've already done this), note the deprecations in the changelogs and urge users to upgrade their code, then drop the deprectated stuff with the next major version.

Either that or make it very clear in the docs that this package does not follow SemVer and users should lock their minor versions.

@thePanz In those cases, isn't the @ the only thing preventing an Exception from being generated, because it temporarily sets error_reporting to 0? I guess that could be a viable alternative as well. Trigger deprecation errors, but use @trigger_error.

[ezimuel]

@thePanz thanks, also in Zend Framework, the project that I worked on, uses it. For instance, in zend-servicemanager, see here.

@AegirLeet I feel that using only @deprecated tag in a comment is not enough. I really like the idea of having E_USER_DEPRECATED by the PHP language itself (that was the main reason for that). Anyway, I see also your point with the usage of Laravel as well.

@thePanz interesting the idea of suppress error with @trigger_error. I need to investigate more on this.

[ezimuel]

@deguif I just added the missing 'Elasticsearch\Endpoints\Cluster\Nodes\Info and 'Elasticsearch\Endpoints\Cluster\Nodes\Stats classes. They are now part of Nodes namespace.
Can you try now? Thanks!

[deguif]

@ezimuel I don't see any changes in your PR, did you push the changes?

[ezimuel]

@deguif sorry, I sent now. I think it's better to call it a day.

[deguif]

@ezimuel that's working ;)
No more errors on the tests related to class not found.

Thanks for that.

[ezimuel]

@deguif you are welcome and thanks for your kindly support!

[iget-master]

I understand your reasons. Maybe it's my fault to rely too mutch on
versioning, I should be more restrictive.

Em seg, 25 de nov de 2019 14:21, Enrico Zimuel [e-mail]
escreveu:

@deguif https://github.com/deguif you are welcome and thanks for your
kindly support!

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/elastic/elasticsearch-php/issues/967?email_source=notifications&email_token=ACH7OT6MA5NUBTHMRUUHAG3QVQCRZA5CNFSM4JPFZBQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFDE3TI#issuecomment-558255565,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/ACH7OT6OGFRS2WNDP2W4X2TQVQCRZANCNFSM4JPFZBQQ
.

[ezimuel]

I just released 7.4.1 that solves the BC break related to the endpoint class naming and it solves also the trigger_error() issue https://github.com/babenkoivan/scout-elasticsearch-driver/issues/297 using the suppress error @ operator.