Skip to content

Sync some classsynopses with stubs #3367

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

Merged
merged 3 commits into from
May 20, 2024
Merged

Conversation

kocsismate
Copy link
Member

No description provided.

@kocsismate kocsismate requested a review from Girgias May 3, 2024 20:44
@kocsismate kocsismate requested a review from nielsdos as a code owner May 3, 2024 20:44
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.

@haszi are the ID changes for the class constants an issue for linking?

@haszi
Copy link
Contributor

haszi commented May 6, 2024

@haszi are the ID changes for the class constants an issue for linking?

Unfortunately they are. Class constant IDs are linking to their declaration only if they are in the classname.constants.constant_name format and non-class constants in the constant.constant_name format.

@kocsismate
Copy link
Member Author

kocsismate commented May 8, 2024

Ah, it has just come to my mind what the initial problem was: since these classes are called the same way as the extension, the ID on the global constant page collides with the class constant list ID. E.g. in case of FFI, both are ffi.constants...

Do you have an idea what the solution could be? I guess the most disruptive one is to use a different pattern for either global constants or class constants, like class-name.class.constants.constant-name... But I hope there's a solution which requires less changes.

@Girgias
Copy link
Member

Girgias commented May 9, 2024

I'm confused?

Surely a non-class constant cannot conflict as the former must be constant.ffi.constant_name and for the class ones it is ffi.constants.constant_name or am I missing something here?

@kocsismate
Copy link
Member Author

Surely a non-class constant cannot conflict as the former must be constant.ffi.constant_name and for the class ones it is ffi.constants.constant_name or am I missing something here?

Not the ID of the constants themselves, but the ID of the constant container (?) collides. See

<appendix xml:id="ffi.constants" xmlns="http://docbook.org/ns/docbook" xmlns:xlink="http://www.w3.org/1999/xlink">
. and
<section xml:id="ffi-ffi.constants">
That's why I had to override the class constant IDs.

@Girgias
Copy link
Member

Girgias commented May 10, 2024

Could we maybe just remove that file and appendix? If the extension doesn't provide any constants I'm not sure it is super useful to have the page in the first place?

@kocsismate
Copy link
Member Author

Could we maybe just remove that file and appendix? If the extension doesn't provide any constants I'm not sure it is super useful to have the page in the first place?

That may work in these specific cases, but we should rather solve the general issue I think 🤔 because the generator should produce valid code

@haszi
Copy link
Contributor

haszi commented May 10, 2024

That may work in these specific cases, but we should rather solve the general issue I think 🤔 because the generator should produce valid code

IDs of classes are already in the class.class_name format so maybe the sections within these elements could use the class.class_name.section_name format (e.g. class.ffi.constants for the constants of the FFI class).

Edit:
Jut to clarify: I'm strictly referring to the IDs of the containing elements (e.g. <appendix>, <section>) and not the constants' (ie. <varlistentry>'s) IDs as making the latter change to all classes would be a massive undertaking.

@kocsismate
Copy link
Member Author

Just to clarify: I'm strictly referring to the IDs of the containing elements (e.g. ,

) and not the constants' (ie. 's) IDs as making the latter change to all classes would be a massive undertaking.

Yes, I'm all for going with the fix which requires the least amount of change, but unfortunately, as far as I remember, the constant IDs have to start with the container ID. 🤔 Let me check if this is still the case.

@kocsismate
Copy link
Member Author

Let me check if this is still the case.

Looks like, it's not 🥳 I'm not sure what I remember about, maybe I just mixed up something.

@haszi
Copy link
Contributor

haszi commented May 15, 2024

Could you drop the leading and trailing hyphens from FFI::__BIGGEST_ALIGNMENT__'s ID?
Leading and trailing hyphens were dropped from IDs as suggested here and here (and implemented in PhD here).

@kocsismate
Copy link
Member Author

There are a few properties which start with a double hyphen, like SoapClient:: $__soap_fault. Do we want to remove them as well or are we fine with them?

@kocsismate
Copy link
Member Author

@haszi I updated gen_stub to generate the correct fieldsynopsis IDs: #3367 (I can remove the property related change if we don't need it)

@haszi
Copy link
Contributor

haszi commented May 16, 2024

There are a few properties which start with a double hyphen, like SoapClient:: $__soap_fault. Do we want to remove them as well or are we fine with them?

Since we already removed them from constant IDs and they aren't part of the method IDs either, I think it would make sense to remove them from all IDs. From a quick look (grep -rE 'xml:id=".*(__|--|\.-|\._).*"'), there are only 29 of these in doc-en anyway.

kocsismate added a commit to php/php-src that referenced this pull request May 17, 2024
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 this looks good, @haszi can you confirm?

@haszi
Copy link
Contributor

haszi commented May 20, 2024

I think this looks good, @haszi can you confirm?

Looks good to me.

@Girgias Girgias merged commit 2a8b2f1 into php:master May 20, 2024
2 checks passed
@kocsismate kocsismate deleted the stub-sync3 branch May 20, 2024 19:03
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.

3 participants