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.

Sign up for GitHub

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

Jump to bottom

Review deprecations triggering #1823

Merged
merged 3 commits into from
Oct 26, 2020
Merged

Review deprecations triggering #1823

merged 3 commits into from
Oct 26, 2020

Conversation

deguif
Copy link
Collaborator

@deguif deguif commented Oct 21, 2020 •
edited
Loading

See elastic/elasticsearch-php#973 for details about why unsilenced deprecations can cause issues.
Symfony link related in the elasticsearch issue is outdated as it points to current version which use trigger_deprecation, see this link instead.

Here what is stated in the symfony documentation:

Without the @-silencing operator, users would need to opt-out from deprecation notices. Silencing 
swaps this behavior and allows users to opt-in when they are ready to cope with them (by adding 
a custom error handler like the one used by the Web Debug Toolbar or by the PHPUnit bridge).

I added symfony/deprecation-contracts to better handle deprecations (they take care of silencing the deprecations).

@deguif deguif mentioned this pull request Oct 22, 2020
Merged
deguif
deguif commented Oct 22, 2020
View reviewed changes
src/Query/Match.php Outdated
@@ -2,7 +2,7 @@

namespace Elastica\Query;

\trigger_error('Elastica\Query\Match is deprecated. Use Elastica\Query\MatchQuery instead. From PHP 8 match is reserved word and this class will be removed in further Elastica releases', \E_USER_DEPRECATED);
trigger_deprecation('ruflin/elastica', '7.0.1', 'The "%s" class is deprecated, use "%s" instead. "match" is a reserved keyword starting from PHP 8.0. It will be removed in 8.0.', Match::class, MatchQuery::class);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe 7.1.0 instead? I don't know which next version you have in mind @ruflin

Copy link
Owner

Choose a reason for hiding this comment

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

Lets put 7.1.0 for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perfect, I'm updating it and previously merged deprecations

@deguif deguif force-pushed the deprecations branch 2 times, most recently from fd4a6c6 to 3adef71 Compare October 23, 2020 09:51
@deguif deguif mentioned this pull request Oct 23, 2020
Merged
ruflin
ruflin reviewed Oct 26, 2020
View reviewed changes
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.

I'm undecided if we should just go with silencing with @ or add the dependency. I'm always a bit torn on adding more dependencies at the same time it is nice to unify.

src/Query/Match.php Outdated
@@ -2,7 +2,7 @@

namespace Elastica\Query;

\trigger_error('Elastica\Query\Match is deprecated. Use Elastica\Query\MatchQuery instead. From PHP 8 match is reserved word and this class will be removed in further Elastica releases', \E_USER_DEPRECATED);
trigger_deprecation('ruflin/elastica', '7.0.1', 'The "%s" class is deprecated, use "%s" instead. "match" is a reserved keyword starting from PHP 8.0. It will be removed in 8.0.', Match::class, MatchQuery::class);
Copy link
Owner

Choose a reason for hiding this comment

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

Lets put 7.1.0 for now.

@deguif
Copy link
Collaborator Author

deguif commented Oct 26, 2020

Yes I added silencing with @ on new PRs which triggers deprecations, but symfony/deprecation-contracts would add a way to enforce proper deprecations messages and would be a very small dependency introduction, but I'm also mitigated with its interests as this can be done with conventions with @trigger ;)

nicolas-grekas
nicolas-grekas approved these changes Oct 26, 2020
View reviewed changes
Copy link

@nicolas-grekas nicolas-grekas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks, I fully support this change!
Silencing the deprecations is a must because this is what makes them opt-in:

  • without the silencing operator, ppl MUST configure their prod to ignore the deprecations: that means ppl are forced to opt-out;
  • with silencing, deprecations are silent, unless one decides to use a tool that gathers them and reports them appropriately (such as Symfony's): this is opt-in.

About symfony/deprecation-contracts, it's been designed to help the community standardize on a very simple and generic signature and implementation. I wish all PHP projects would adopt it. That would help build better tools to display nice deprecation reports.

👍 on my side!

@deguif deguif force-pushed the deprecations branch 2 times, most recently from aa04173 to 2ec4657 Compare October 26, 2020 11:05
@deguif deguif merged commit 4336265 into ruflin:master Oct 26, 2020
@deguif deguif deleted the deprecations branch October 26, 2020 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@ruflin ruflin ruflin approved these changes

Assignees
No one assigned
Labels
hacktoberfest-accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@deguif @nicolas-grekas @ruflin