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

Deprecate unsuffixed processor classes #1893

Merged
merged 3 commits into from
Feb 11, 2021

Conversation

deguif
Copy link
Collaborator

@deguif deguif commented Dec 4, 2020

No description provided.

@deguif deguif force-pushed the deprecate-unsuffixed-processor-classes branch 3 times, most recently from c720a29 to 507d174 Compare December 4, 2020 15:13
@ruflin
Copy link
Owner

ruflin commented Dec 7, 2020

Can you share some background on why you want to suffix them all? Is the plan to suffix all the other assets too like Aggregations (see the other discussion).

@deguif
Copy link
Collaborator Author

deguif commented Dec 7, 2020

Yes, see my comment: #1880 (comment)
I'm starting by processors as that should impact less library users (deprecations triggering) and that would let us see how it's received by them.
The plan I had in mind is to apply to all aggregations / queries classes later (for 7.2.0 for example).
With that, we should be less impacted by new reserved keywords introduced in newer PHP versions (recent example is match query class) and have the same naming strategy for all these classes.

@deguif deguif force-pushed the deprecate-unsuffixed-processor-classes branch 3 times, most recently from da8474d to 1330f6e Compare December 7, 2020 17:35
@ruflin
Copy link
Owner

ruflin commented Dec 8, 2020

I must confess I'm torn on this. I definitively don't like the conflicts but at the same time it seems extreme for 2-3 conflicts to rename ALL classes and with it basically force all users to modify their app at one stage. Instead I would probably more have a default fallback way that we use for the conflicts but keep as is.

If I understand this change right and we follow through with it, it will affect the majority of classes in the library?

@deguif
Copy link
Collaborator Author

deguif commented Dec 18, 2020

Yes at least, for the main ones (and most subject to colliding with reserved keywords) in the Query, Aggregation and Processor namespaces.
It's done in a backward compatible way so should have no impact for library users (just deprecations).

There's also the comment of @krewetka regarding class name and their readibility that I 👍 : #1880 (comment)

But that's not a blocker, the goal here is to clean / unify things, this can be ignored (and this PR closed) if we're not agreeing on this point ;)

@ruflin
Copy link
Owner

ruflin commented Dec 18, 2020

@thePanz Would be great to get one more opinion on this, could you chime in?

@thePanz
Copy link
Collaborator

thePanz commented Dec 22, 2020

@ruflin @deguif I like the approach, each class name is now "self-explanatory"... the most relevant example is the "Date" processor, being renamed to DateProcessor makes things more clean IMHO

Just a niptick: can we remove @return $this when as the method's signature is returning self?

Copy link
Owner

@ruflin ruflin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deguif @thePanz Ok, lets move forward with this.

@deguif
Copy link
Collaborator Author

deguif commented Jan 6, 2021

Just a niptick: can we remove @return $this when as the method's signature is returning self?

We can but in case of inheritance this allow to properly infer the right type. This should be solved with static type-hint with PHP 8.0 ;)

@deguif
Copy link
Collaborator Author

deguif commented Jan 17, 2021

May I merge this one ?

@ruflin
Copy link
Owner

ruflin commented Jan 18, 2021

@deguif Go for it.

@deguif deguif force-pushed the deprecate-unsuffixed-processor-classes branch from 1330f6e to 9875a54 Compare February 11, 2021 13:20
@deguif deguif merged commit ec41f0d into ruflin:master Feb 11, 2021
@deguif deguif deleted the deprecate-unsuffixed-processor-classes branch February 11, 2021 13:41
@deguif deguif self-assigned this Dec 7, 2021
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