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

Generate deprecation warning box for the manual #14938

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kocsismate
Copy link
Member

By using the information available by the newly added Deprecated property, the deprecation warning boxes in the manual can now be generated when not exists. (if it already exists, the box may contain different alternative suggestion than what's written in the stub, so this case is not handled).

@kocsismate kocsismate marked this pull request as ready for review July 12, 2024 19:49
$refsynopsisDiv = $doc->createElement('refsynopsisdiv');
$deprecationEntity = $doc->createEntityReference(
'warn.deprecated.' . (isset($aliasMap[$this->name->__toString()]) ? 'alias' : 'function-') .
str_replace(".", "-", $this->deprecatedSince) . "-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.

@TimWolla The since property currently doesn't contain the patch version. However, theoretically it's possible to deprecate symbols in patch versions as well... So I'm wondering whether the .0 suffix could be added to the property value?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, currently, 3rd party extensions shouldn't really use Deprecated attributes... because currently I have to assume that the value of the since property relates to PHP versions, and not the version of the extension itself.

Copy link
Member

Choose a reason for hiding this comment

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

However, theoretically it's possible to deprecate symbols in patch versions as well... So I'm wondering whether the .0 suffix could be added to the property value?

I don't see a value-add in that. Deprecations in patch version should be very rare. As an example, if folks convert deprecations to Exceptions this would cause a break. Yes, folks should not do that, but they do.

Pretending that the deprecation was there in the .0 version is a good enough approximation, because only the latest patch version of a branch is supported anyways.

Copy link
Member

Choose a reason for hiding this comment

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

Also, currently, 3rd party extensions shouldn't really use Deprecated attributes... because currently I have to assume that the value of the since property relates to PHP versions, and not the version of the extension itself.

see: https://externals.io/message/123397#124367

if ($funcInfo->deprecatedSince) {
foreach ($doc->getElementsByTagName("refentry") as $refentryElement) {
foreach ($refentryElement->getElementsByTagName("refnamediv") as $refnameDivElement) {
if (count($refnameDivElement->getElementsByTagName("refname")) !== 1) {
Copy link
Member Author

Choose a reason for hiding this comment

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

The warning saying This function has been DEPRECATED as of ... doesn't make sense on pages where multiple functions/methods are documented, so I skip the automatic addition of the box in such cases.

'warn.deprecated.' . (isset($aliasMap[$this->name->__toString()]) ? 'alias' : 'function-') .
str_replace(".", "-", $this->deprecatedSince) . "-0"
);
$refsynopsisDiv->appendChild(new DOMText("\n "));
Copy link
Member

Choose a reason for hiding this comment

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

You can actually simplify DOMText insertions as follows:

Suggested change
$refsynopsisDiv->appendChild(new DOMText("\n "));
$refsynopsisDiv->append("\n ");

or even combine the 3 appendChilds into one, but I guess you didn't do it because it would be a long line or would have to be multilined anyway.

build/gen_stub.php Outdated Show resolved Hide resolved
@Girgias
Copy link
Member

Girgias commented Jul 13, 2024

I am wondering if we shouldn't move this to PhD, as in use the information of the version.xml files with the deprecated and/or removed attributes attached to the <function> tags to generate this box automatically?

This information is already used to generate the ToC and move deprecated functions/methods into a new box:
image

This would lower the maintenance burden for translations, and have the information be more accurate from the get go, changelogs would still need to be manually added however

@Girgias
Copy link
Member

Girgias commented Jul 13, 2024

Although for this to work, we would need gen_stub to properly generate the version.xml file in the first place

@kocsismate
Copy link
Member Author

kocsismate commented Jul 14, 2024

I am wondering if we shouldn't move this to PhD, as in use the information of the version.xml files with the deprecated and/or removed attributes attached to the tags to generate this box automatically?

Yes, and there's another problem with the alternative suggestion: this information isn't available for phd. And as I found out, not even gen_stub.php can always generate this properly. :( On one hand, it's because the XML markup is missing when a symbol is referenced (e.g.: use IntlDateFormatter::format() instead). On the other hand, sometimes the manual should explain the problem in more detail than what's written in the stub (funnily enough there is this message: visit the php.net documentation for various alternatives. This one shouldn't be added in the warning box 😅 ).

That's why I went with what I wrote in the description: the replacement only happens if there is no similar box added yet (then the "skeleton" text can be fine-tuned afterwards).

Although for this to work, we would need gen_stub to properly generate the version.xml file in the first place

Yes, it would be awesome if it did! But then we should add the version numbers for all functions... Uh, it sounds like huge pain.

@cmb69
Copy link
Member

cmb69 commented Jul 19, 2024

Although for this to work, we would need gen_stub to properly generate the version.xml file in the first place

Yes, it would be awesome if it did! But then we should add the version numbers for all functions... Uh, it sounds like huge pain.

If it's a huge pain, maybe leave it as is for now. :)

@TimWolla
Copy link
Member

Yes, it would be awesome if it did! But then we should add the version numbers for all functions... Uh, it sounds like huge pain.

The $since property should be filled in for all currently-deprecated-but-not-yet-removed functions as part of the move to #[\Deprecated] from @deprecated. Just the $message is missing for some.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants