search

markstory / sphinxcontrib-phpdomain

A PHP domain for sphinx. Allows you to annotate PHP objects in your sphinx docs.
Other
19 stars 15 forks source link

Reference to namespaced class property with `~` prefix and without specifying namespace does not produce link #44

Closed n-peugnet closed 1 year ago

n-peugnet commented 1 year ago

Basically, with this diff to the tests, the second reference added will not produce a link, whereas the two other will:

diff --git a/test/test_doc.rst b/test/test_doc.rst
index 6dee8fc..cbd20f9 100644
--- a/test/test_doc.rst
+++ b/test/test_doc.rst
@@ -172,6 +172,8 @@ Test Case - Global symbols with no namespaces

 :php:func:`DateTime::$testattr`

+:php:func:`~DateTime::$testattr`
+
 :php:func:`OtherClass::update`

 :php:attr:`OtherClass::$nonIndentedAttribute`
@@ -325,6 +327,8 @@ Test Case - not including namespace

 :php:attr:`LibraryClass::$property`

+:php:attr:`~LibraryClass::$property`
+
 :php:const:`LibraryClass::TEST_CONST`

 :php:class:`LibraryName\\OtherClass`
@@ -378,6 +382,8 @@ Test Case - global access

 :php:attr:`LibraryName\\LibraryClass::$property`

+:php:attr:`~LibraryName\\LibraryClass::$property`
+
 :php:const:`LibraryName\\LibraryClass::TEST_CONST`

 :php:const:`LibraryName\\NS_CONST`
n-peugnet commented 1 year ago

I identified the source of this bug.

It is because this block is not executed since after the evaluation of the ~ prefix the title is only $property: https://github.com/markstory/sphinxcontrib-phpdomain/blob/2d06842f100846505084b6aea9bf7b570c233037/sphinxcontrib/phpdomain.py#L518-L520

Which leads to modname and clsname being None in resolve_xref() and find_obj() to return None: https://github.com/markstory/sphinxcontrib-phpdomain/blob/2d06842f100846505084b6aea9bf7b570c233037/sphinxcontrib/phpdomain.py#L701-L710

TBH I don't get why the offending line is there in the first place. I don't see any reason for not including these two values when the title starts with $. To filter out global variable? Then this check is incorrect, and even then, why check for global variables and not global functions?

I tried to remove the if and always add the namespace and class values and I didn't find any issues. But as the tests are not very strict I could easily have missed some regressions.