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

IBX-4929: ContentService: Fix @see usage #219

Merged
merged 1 commit into from
Mar 29, 2023
Merged

Conversation

adriendupuis
Copy link
Contributor

@adriendupuis adriendupuis commented Mar 16, 2023

Question Answer
JIRA issue IBX-4929
Type improvement
Target Ibexa version v4.0
BC breaks yes/no

Fix @see usage for phpDocumentor 3.3.1

  • The parenthesis are mandatory for the reference to be seen as a function.
  • The className shouldn't but seems mandatory too.

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Provided automated test coverage.
  • Checked that target branch is set correctly (main for features, the oldest supported for bugs).
  • Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
  • Asked for a review (ping @ibexa/engineering).

For phpDocumentor 3.3.1
- the parenthesis are mandatory for the reference to be seen as a function
- the className shouldn't but seems mandatory too.
@sonarcloud
Copy link

sonarcloud bot commented Mar 16, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

The parenthesis are mandatory for the reference to be seen as a function.

This is bad. It's semantically wrong from the software engineering point of view. You're referencing a method, not its call. If there's no way to work around it, maybe we should drop this arcane tool?

@adriendupuis
Copy link
Contributor Author

adriendupuis commented Mar 29, 2023

The parenthesis are mandatory for the reference to be seen as a function.

This is bad. It's semantically wrong from the software engineering point of view. You're referencing a method, not its call. If there's no way to work around it, maybe we should drop this arcane tool?

@alongosz, I agree with the semantic problem. The parenthesis are not part of the method name.

phpDocumentor use () to show that it's a method also in methods menu list.
It's a convention that can be observed elsewhere like on W3Schools (I'm not fan of this website but that's not the subject): https://www.w3schools.com/php/func_array_map.asp

From my POV, this ugly convention might come from PHP itself.
PHP doesn't see Class::method as a reference to a method and will look for a constant.
You can't write something like $m=MyClass::staticMethod; $m();.
But, you can stupidly write:

class Test
{
    public const sameName = 'constant';

    public static function sameName()
    {
        return func_get_args();
    }
}
var_dump(Test::sameName, Test::sameName('method'));
$v='Test::sameName';
var_dump(constant($v), $v('method'));

In this example, () could help to solve the homonymy and precise that the subject is the method, not the constant.

From my POV, this parenthesis convention is acceptable.

About alternatives to phpDocumentor, I haven't test much yet (there isn't much alternatives anyway).

  • Doxygen is a nuclear power plant. It takes hours where other takes minutes but it has a beautiful output with inheritance diagrams (I guess they are the ones consuming time and could be disabled). Navigation in the documentation is not as simple as phpDocumentor. It doesn't use the () convention in tis output. It wrongly renders inline @see as a block @see and need the () in the input.
  • ApiGen sadly doesn't render the @see while it has a interesting default layout. Navigation is less straight forward than phpDocumentor but a bit better than Doxygen. It doesn't use the () convention in its output.

I'm looking for others.

For now, phpDocumentor is the easiest to use with automation, the easiest to stylize, with a look & feel similar to the REST API Reference.

@alongosz alongosz dismissed their stale review March 29, 2023 08:15

The decision has been overridden

Copy link
Contributor

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

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

I agree with @alongosz about the semantic meaning, but given that it is our tool of choice and the "impact" of the change is minimal, I'm ok with it 😅

@webhdx
Copy link
Contributor

webhdx commented Mar 29, 2023

@adriendupuis Since we've agreed to use phpDoc in the past we need to comply with its syntax and not argue wether it's correct or not. I believe we may have issues like this in many more places.

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

From my POV, this ugly convention might come from PHP itself.
PHP doesn't see Class::method as a reference to a method and will look for a constant.
You can't write something like $m=MyClass::staticMethod; $m();.

This is very good argument 👍

@alongosz alongosz merged commit 15abb12 into main Mar 29, 2023
@alongosz alongosz deleted the adriendupuis-patch-3 branch March 29, 2023 08:58
@alongosz
Copy link
Member

Thank you @adriendupuis 🎉

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.

7 participants