Remove aggregation deprecations #2138
Conversation
franmomu
force-pushed
the
remove_agg_deprecations
branch
from
fa39e78
to
c05befc
Compare
| { | ||
| parent::__construct($name); | ||
|
|
||
| if (null !== $bucketsPath) { |
There was a problem hiding this comment.
This one we did not deprecate.
One thing I've seen in the past is that an object was created when the bucketPath did not exist yet and was set at a later stage (just an example). Is this a use case we want to support or on this scenario we except the object to be created when all info is available?
There was a problem hiding this comment.
Yeah, I would deprecate them in 7.x.
When working with ES, these parameters are required:
https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-pipeline-bucket-script-aggregation.html
https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-pipeline-bucket-selector-aggregation.html
Since they are required, I think it makes sense to not be able to create a BucketScript instance with invalid data.
IMHO we should avoid supporting those scenarios where these classes can contain invalid data (in some of them we can even mark them as immutable), it simplifies the code and ease the maintenance since these objects would always be in a valid state.
There was a problem hiding this comment.
Lets call this a decision that all required params have to be set when creating an object.
Can you open a PR against 7.x with a deprecation?
| { | ||
| parent::__construct($name); | ||
|
|
||
| if (null !== $bucketsPath) { |
There was a problem hiding this comment.
Same as above. Do we need to deprecate it in 7.x branch if we go down this path?
|
As discussed above, would be great to open PR's against 7.x with the deprecations. |
I would go a step further (in another PR) and remove setters or make them private for mandatory data passed in the constructor like
setBucketsPath()orsetScript().