-
Notifications
You must be signed in to change notification settings - Fork 25
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
Move to a modern version of reflection docblock #140
Move to a modern version of reflection docblock #140
Conversation
…r used by ModelAsController
…ts with PHPUnit 9.5 supported by Silverstripe 4.10.0
@robbieaverill hate to be a pest on this but do you know who is active as a maintainer on this repo now? This pull is a bit of a blocker if you want to use PHPUnit 9 in your project with this module. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I think the version bump would either require a minor or major release in this module
Ya I would think it's at least a minor bump, the two versions of phpdocumentor/reflection-docblock are pretty large gaps plus the base supported PHP version is also increased (more inline with modern Silverstripe). |
Moved pageClassesCache into the AbstractTagGenerator class and made it protected
…g controller_name on the page type
I've pushed some changes, see above for the one outstanding issue |
Note about |
LGTM now. I want to give it a few manual tests before merging and tagging a new release. I'm guessing this would be release line 4.x, given the updates. |
LGTM, it might be safest as a new major version release yeah. |
A quick test shows all classes being prefixed with a
That does not sound correct to me |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All @method
tag classes are prefixed with a \
, this causes the class paths to be wrong.
$docBlock->setText('Class \\' . $this->className); | ||
$summary = $docBlock->getSummary(); | ||
if (!$summary) { | ||
$summary = sprintf('Class \\%s', $this->className); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it might be here, something goes wrong and all classes are prefixed with a \
@Firesphere this appears to be the behavior in the current version as well, and technically would make sense I think otherwise it would be relative to the current namespace of the file (at least if you tried to use that same path in PHP). VSCode using intellephence does seem to properly handle this, I'm curious how does your ide of choice handle it? |
PHPStorm does not handle this properly, and I've not seen it do that before, as it shouldn't include the namespace, as far as I remember... |
Hmm I think that may cause more issues, particularly when it comes to aliased classes in the use statement. It looks like at least based on the PHPDocumentor docs that class names in types can be the FQCN or the local, but to do the local we'd need to parse the document I would think to figure out the alias if present as a use statement. |
This has been discussed in a different PR, where we decided against both, IIRC. |
@Firesphere I think the issue you are seeing is because it's not resolving to the full path to the class. It really should be |
Yes, it's literally above, you could probably determine my DB/Relations from the example there :) |
hmmm I didn't not observe that behavior using a model I have it generated as I would expect. Is it possible PHPStorm is adjusting them after this branch generates them?
My local composer.lock has the following for PHPDocumentor's dependencies:
Perhaps there is a miss-match in dependencies as well that maybe causing the problem? |
Right, I see the problem, I think. If the class is already in the |
Ah do you have the |
If so I believe that's the source of the issue, I'm not sure that will work properly with the newer versions of phpDocumentor. There maybe something with the context that can get passed into $this->tagFactory->create() but I'm not 100% sure how that works. If I pass a context created from the It makes me wonder if we should just remove that config option for this new version? It sounds like PHPStorm and most ides (from a quick google) should properly support the FQNs. |
We can't remove that option, sadly/probably. Reason is, that if people go in to the file, and import the class with a use case, the IDE like PHPStorm, will shorten it. A build will then lengthen it again, or break it. Neither of those is a desirable outcome. |
…bjectAnnotator.use_short_name config to work with the new version of reflection docblock
@Firesphere sorry for the slow follow up I've added a new resolver that seems to address the issue you called out. Let me know 😄 |
While at it, could you please update the following? This will make it PHP 8.1 compatible in a single go |
I have no other comments, but would like that minor change to be added in this PR and make it all go to a new point release |
I've replaced the line you referenced with your line @Firesphere (with |
This pull request incorporates changes from #139 but more importantly switches to phpdocumentor/reflection-docblock ^5.2 to address compatibility with PHPUnit ^9.5 that Silverstripe 4.10 supports. I believe this may also break compatibility with Silverstripe projects using PHPUnit 5.7 because of conflicting dependencies between the two versions.