Skip to content

Various fixes for even more interfaces#2560

Closed
mallardduck wants to merge 4 commits into
php:masterfrom
mallardduck:fix-more-interfaces
Closed

Various fixes for even more interfaces#2560
mallardduck wants to merge 4 commits into
php:masterfrom
mallardduck:fix-more-interfaces

Conversation

@mallardduck
Copy link
Copy Markdown
Contributor

Fixes:

  • Missing spaces on: a few predefined interfaces,
  • Interfaces displaying as classes: cmark, json, pthreads

@Girgias
Copy link
Copy Markdown
Member

Girgias commented Jul 5, 2023

@kocsismate (knowing that you're busy) do you know why the JSON and /language/ files were not updated by your previous stub sync? Is this an issue in our stubs or just that you forgot?

@@ -35,9 +35,7 @@
</ooclass>

<classsynopsisinfo>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
<classsynopsisinfo>
<classsynopsis class="interface">

Isn't this missing, or am I not understanding how the new markup should look?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So for the new markup generation to kick in, we want it to call format_classsynopsisinfo_ooclass_classname - which means we need a ooclass inside of a classsynopsisinfo.

Then once that's being called we also need the class attr set to interface - which changes the render from calling it a class to use interface. Similarly, when that attr is set this way the oointerface that's being extended/implemented will adjust for the correct verb.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay, I'm going to group thoughts in the issues I opened to try and centralise stuff instead of me commenting left and right.

@Girgias
Copy link
Copy Markdown
Member

Girgias commented Jul 5, 2023

Okay, I'm not sure I actually really like this new expected format for interfaces, it is very confusing.

Can't we do something like:

   <classsynopsis class="interface">
    <oointerface>
     <interfacename>Throwable</interfacename>
    </oointerface>

    <classsynopsisinfo>
     <oointerface>
      <interfacename>Throwable</interfacename>
     </oointerface>

     <oointerface>
      <modifier>extends</modifier>
      <interfacename>Stringable</interfacename>
     </oointerface>

     <oointerface>
      <modifier>extends</modifier>
      <interfacename>SecondInterface</interfacename>
     </oointerface>
    </classsynopsisinfo>

    <classsynopsisinfo role="comment">&Methods;</classsynopsisinfo>
    <xi:include xpointer="xmlns(db=http://docbook.org/ns/docbook) xpointer(id('class.throwable')/db:refentry/db:refsect1[@role='description']/descendant::db:methodsynopsis[@role='Throwable'])">
     <xi:fallback/>
    </xi:include>

    <classsynopsisinfo role="comment">&InheritedMethods;</classsynopsisinfo>
    <xi:include xpointer="xmlns(db=http://docbook.org/ns/docbook) xpointer(id('class.stringable')/db:refentry/db:refsect1[@role='description']/descendant::db:methodsynopsis[@role='Stringable'])">
     <xi:fallback/>
    </xi:include>
   </classsynopsis>

And where PhD collapses multiple oointerface tags that have an extends modifier into one list? As far as I can see this should be valid according to the DocBooks DTD

References:

@kocsismate
Copy link
Copy Markdown
Member

do you know why the JSON and /language/ files were not updated by your previous stub sync? Is this an issue in our stubs or just that you forgot?

Hmm, before committing my changes, I had to filter out a few files (which have unrelated diffs), and probably I missed JsonSerializable. But I'm not sure about why interfaces in the language directory didn't show up among the changes. That may be a bug in gen_stub.php 🤔

<oointerface>
<interfacename>BackedEnum</interfacename>
</oointerface>
<ooclass><classname>BackedEnum</classname></ooclass>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please do not do commit this change on its own since it's contrary to the output of gen_stub.php (the script which generates these markups). Please rather fix the script itself.

Copy link
Copy Markdown
Contributor Author

@mallardduck mallardduck Jul 7, 2023

Choose a reason for hiding this comment

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

@kocsismate Are there any docs or wiki pages somewhere detailing how to work with the gen_stub.php script? I have ran it before in the past on php-src but am not sure how I would run that script for the extensions? Oh my fault - for some reason I forgot this PR was attempting to fix both extension refs and language docs.

@Girgias
Copy link
Copy Markdown
Member

Girgias commented Jul 7, 2023

Right, @kocsismate I think the current way we render the class synopsis is kinda bad, see #2563 (and the related PhD PR) to see what the new markup would look like.

@mallardduck
Copy link
Copy Markdown
Contributor Author

Going to close this per the new direction.

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