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

Extensions on Controller doesn't work as expected #126

Closed
madmatt opened this issue Sep 25, 2019 · 6 comments
Closed

Extensions on Controller doesn't work as expected #126

madmatt opened this issue Sep 25, 2019 · 6 comments

Comments

@madmatt
Copy link

madmatt commented Sep 25, 2019

Having an extension that extends SilverStripe\Control\Controller appears to cut off classes.

An example docblock that gets generated is:
@property \SilverStripe\Admin\SecurityAdmin|\SilverStripe\GraphQL\Controller|\SilverStripe\TestSession\TestSessionController|\SilverShop\Comparison\Pagetypes\ProductComparisonPageController|\SilverShop\Admin\ProductCatalogAdmin|\SilverShop\Cart\ShoppingCartController|\SilverShop\Page\AccountPageController|\SilverShop\Page\CheckoutPageController|\SilverShop\Page\ProductCategoryController|\SilverShop\Page\ProductController|\SilverShop\Discounts\Admin\DiscountModelAdmin|\SilverStripe\CMS\Controllers\CMSMain|\SilverStripe\CMS\Controllers\CMSPageSettingsController|\SilverStripe\CMS\Controllers\ContentController|\SilverStripe\CMS\Controllers\ModelAsController|\SilverStripe\Control\Controller|\SilverStripe\Dev\DevBuildController|\App\Extensions\ControllerExtension $owner

I would have expected either one of two things to happen:

  • Only the direct extended class gets added as $owner (e.g. @property SilverStripe\Control\Controller|App\Extensions\ControllerExtension $owner)
  • Every single sub-class of Controller gets added (there are a lot more than what's listed here).

I'm not sure which option is better, but this seems to be the worst of both worlds? I haven't had a chance to investigate yet, but is there thought on which solution is better?

Alternatively, maybe I'm just confused how this works, but e.g. PageController isn't there, so there are entire hierarchies missing.

@Firesphere
Copy link
Member

I've noticed the same behaviour for extensions on DataObject, it takes all children, instead of what it's actually extending at the base.

@Firesphere
Copy link
Member

Feel free to make a PR @madmatt :D Also, while at it, do you want to upgrade the PHPDocumentor library pretty please? :)

@madmatt
Copy link
Author

madmatt commented Sep 29, 2019

I was trying to figure out what the best way to fix this is... what do you think @Firesphere?

IMO, we should look at the YML and add hints for every class that the extension directly extends (e.g. it might show more than one class, but will only ever show classes that it directly extends. I think this best signals the intent of the extension (in that if you call a method that exists on HomePageController and you only extend Page then you should rightly see a warning from your IDE...

Thoughts welcome before I go diving into this on the next hackday I get a chance to participate in 😉

@Firesphere
Copy link
Member

@Firesphere
Copy link
Member

I think this has been adressed in the PR. @madmatt , can you confirm dev-master does not have the problem anymore?

@Firesphere
Copy link
Member

Closing, I'm fairly certain this has been resolved, and we've not heard back in 2+ years.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants