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

Replace @deprecated by #[\Deprecated] for internal functions / class constants #14750

Merged
merged 1 commit into from
Jul 10, 2024
Merged

Replace @deprecated by #[\Deprecated] for internal functions / class constants #14750

merged 1 commit into from
Jul 10, 2024

Conversation

TimWolla
Copy link
Member

@TimWolla TimWolla commented Jul 1, 2024

This is spun out of and depends on #11293.

@TimWolla TimWolla mentioned this pull request Jul 1, 2024
Merged
2 tasks
@TimWolla TimWolla marked this pull request as ready for review July 2, 2024 07:49
@TimWolla
Copy link
Member Author

TimWolla commented Jul 2, 2024

In the previous PR there were some unresolved threads from @Girgias remaining (regarding version information / messages). And there was a discussion regarding the formatting of the version within the $since. You can find it here: #11293 (comment)

nielsdos
nielsdos reviewed Jul 4, 2024
View reviewed changes
ext/libxml/libxml.stub.php Outdated
@@ -172,7 +172,7 @@ function libxml_get_errors(): array {}

function libxml_clear_errors(): void {}

/** @deprecated */
#[\Deprecated(since: '8.0')]
Copy link
Member

Choose a reason for hiding this comment

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

reason: because entity loading is disabled by default since PHP 8.0.

Copy link
Member Author

Choose a reason for hiding this comment

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

@nielsdos Would libxml_disable_entity_loader(false) reenable it or not?

Copy link
Member

@nielsdos nielsdos Jul 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes but only when also passing LIBXML_NOENT to the parsing options of the XML extensions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand that external entities are dangerous, but the deprecation without any alternative sounds wrong.

Anyway: Do you have a specific wording suggestion here? Best to suggest it using GitHub's suggestion feature.

Copy link
Member

Choose a reason for hiding this comment

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

but the deprecation without any alternative sounds wrong.

You can still use them though, this was necessary to combat shit defaults in libxml2 < v2.9. Since PHP 8.0 we require at least libxml2 v2.9 and so the problem went away.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[\Deprecated(since: '8.0')]
#[\Deprecated(message: 'External entity loading is disabled by default', since: '8.0')]

Copy link
Member Author

Choose a reason for hiding this comment

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

You can still use them though.

I understand this is not the correct place for this discussion, but does it require the use of this deprecated function to enable them or will this happen automatically when calling libxml_set_external_entity_loader() or so? If the former, then this doesn't really feel like still being able to use them.

Regarding the change: Thanks, applied (with s/External/as external/ to better fit the grammar).

Copy link
Member

Choose a reason for hiding this comment

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

I understand this is not the correct place for this discussion, but does it require the use of this deprecated function to enable them

No you don't have to use this. You have to pass the LIBXML_NOENT option to an XML parser. libxml_set_external_entity_loader() is orthogonal to this

Girgias
Girgias reviewed Jul 7, 2024
View reviewed changes
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

I think I've covered all the reasonable alternatives now.

ext/intl/php_intl.stub.php Outdated Show resolved Hide resolved
ext/intl/php_intl.stub.php Outdated Show resolved Hide resolved
ext/openssl/openssl.stub.php Outdated Show resolved Hide resolved
ext/openssl/openssl.stub.php Outdated Show resolved Hide resolved
ext/openssl/openssl.stub.php Outdated Show resolved Hide resolved
ext/enchant/enchant.stub.php Outdated Show resolved Hide resolved
ext/date/php_date.stub.php Outdated Show resolved Hide resolved
ext/date/php_date.stub.php Outdated Show resolved Hide resolved
ext/date/php_date.stub.php Outdated Show resolved Hide resolved
ext/date/php_date.stub.php Outdated Show resolved Hide resolved
@TimWolla
Copy link
Member Author

TimWolla commented Jul 8, 2024

I think I've covered all the reasonable alternatives now.

Thanks, all applied and then slightly adjusted for formatting:

  • Moved the since parameter first in the stubs, because it is shorter.
  • Lowercase use in the message.
  • Use consistent phrasing for “use the IntlDateFormatter::format() method instead” 👉 “use IntlDateFormatter::format() instead”

Girgias
Girgias approved these changes Jul 8, 2024
View reviewed changes
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

LGTM from my PoV

@TimWolla
Copy link
Member Author

TimWolla commented Jul 9, 2024

@Girgias I just did a rebase of the stuff you already reviewed and pushed some more commits to add the missing data / to correct some data. All deprecations have a since value filled in (based on the version where the @deprecated was added first or based on the docs depending on what I could find more easily). I've also added a message where the docs already explained what to do succinctly.

@TimWolla TimWolla requested a review from Girgias July 9, 2024 08:40
Girgias
Girgias reviewed Jul 9, 2024
View reviewed changes
ext/date/tests/gmstrftime_variation20.phpt Outdated Show resolved Hide resolved
ext/hash/hash.stub.php Show resolved Hide resolved
ext/zip/tests/zip_entry_open.phpt
@@ -12,12 +12,12 @@ zip_close($zip);

?>
--EXPECTF--
Deprecated: Function zip_open() is deprecated in %s on line %d
Deprecated: Function zip_open() is deprecated since 8.0, use ZipArchive::open() instead in %s on line %d
Copy link
Member

Choose a reason for hiding this comment

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

Okay this is a complete aside, but I just realized that in the documentation, our manual style guide requires use to say "deprecated as of PHP 8.0" instead of "since", not sure if this is something we should match or not, but that's orthogonal to this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think keeping consistency with the property name is valuable here.

@TimWolla @Girgias @nielsdos
Replace @deprecated by #[\Deprecated] for internal functions / cl…
67bb216 
…ass constants

Co-authored-by: Gina Peter Banyard <girgias@php.net>
Co-authored-by: Niels Dossche <7771979+nielsdos@users.noreply.github.com>
@TimWolla TimWolla merged commit 29f98e7 into php:master Jul 10, 2024
11 checks passed
@TimWolla TimWolla deleted the DeprecatedAttribute-apply branch July 10, 2024 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kocsismate kocsismate kocsismate left review comments

@nielsdos nielsdos nielsdos left review comments

@Girgias Girgias Girgias approved these changes

@bukka bukka Awaiting requested review from bukka bukka is a code owner

@devnexen devnexen Awaiting requested review from devnexen devnexen is a code owner

@NattyNarwhal NattyNarwhal Awaiting requested review from NattyNarwhal NattyNarwhal is a code owner

@derickr derickr Awaiting requested review from derickr derickr is a code owner

Assignees
No one assigned
Labels
Extension: date Extension: enchant Extension: hash Extension: intl Extension: libxml Extension: mysqli Extension: odbc Extension: openssl Extension: pgsql Extension: reflection Extension: shmop Extension: spl Extension: standard Extension: zip
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@TimWolla @kocsismate @nielsdos @Girgias