getkirby / getkirby.com

Source code and content for the Kirby website
https://getkirby.com
129 stars 265 forks source link

Flag `@advanced` instead of `@internal` #2264

Open jensscherbl opened 5 months ago

jensscherbl commented 5 months ago

Just stumbled over the example for the system.loadPlugins:after hook in the guide and wanted to dynamically register an extensions based on a config option in a plugin like suggested in the documentation.

// register additional extensions like you would
// directly in the Kirby::plugin() call
kirby()->extend([
   // ...
], kirby()->plugin(...));

However, the \Kirby\Cms\AppPlugins::extend method mentioned in the example is marked as @internal.

/**
* Register all given extensions
*
* @internal
* @param \Kirby\Cms\Plugin $plugin|null The plugin which defined those extensions
*/
public function extend(...): array {...}

return $this->extensions;
}

Now I’m wondering if the guide or the DocBlock annotation is wrong here.

texnixe commented 5 months ago

Unfortunately, it is not clear from the commit or PR that dates back to 2019 why many methods were marked as internal. The methods themselves are missing from the documentation, but nevertheless used in several contexts.

@bastianallgeier @distantnative Could you explain why this was done?

jensscherbl commented 5 months ago

Thanks. I did some digging myself as well and read that @internal annotations were introduced to filter certain methods from the auto-generated reference docs at some point.

Another thought after reporting this was “since the hook callbacks are bound to the Kirby object, I could also use $this->extend(...) instead and tell myself that it qualifies as “internal use”, but it still leaves me with a feeling of uncertainty 😅

From my understanding, if a method is kinda excluded from the public API with this annotation, I shouldn’t rely on it for custom development.

texnixe commented 5 months ago

I did some digging myself as well and read that @internal annotations were introduced to filter certain methods from the auto-generated reference docs at some point.

Exactly, that's what I meant. We filter them out of the docs, only to use them in several examples anyway, which is not really stringent.

bastianallgeier commented 5 months ago

I think this is caused by losing track.

We add @internal for two reasons:

  1. we don't want it to appear in the reference

  2. we are not entirely sure about the implementation yet and we might want to change it later.

  3. is often related to 2. but not always.

There are methods that are simply too much for the object reference. They go too deep and would only be valuable on a very high level and with a lot of additional documentation and explanations.

I think that extend is totally fine like that. I don't see us deprecating or removing this any time soon. But it's also not a method that you would normally need to use. I don't remember how it made it into the hook docs tbh.

texnixe commented 5 months ago

@bastianallgeier However, also methods like $site->visit() are part of the internal methods, although this is needed for multi-language routes and very weird that it's missing from the docs. I think sometimes this classification does not really make sense or seems at least arbitrary.

bastianallgeier commented 5 months ago

@texnixe I think it's sometimes just massively outdated.

distantnative commented 5 months ago

I wouldn't know how we could do this not in any arbitrary way. We have the competing goals of offering our user the best and most detailed information vs. not overwhelming our users (especially new ones) with a myriad of very technical methods - in the end they don't find the important basic ones amidst all those very advanced ones. I think any decision to draw a line - this is in, this is out - will be arbitrary. And will be wrong at times (or actually right, even if one user among hundreds was looking for it) with the need to revise them when we notice that our classification doesn't fit (anymore). I can't see how there could be an exact system of rules to decide.

lukasbestle commented 5 months ago

Maybe it could help to differentiate:

bastianallgeier commented 5 months ago

@lukasbestle I like that suggestion a lot.

jensscherbl commented 5 months ago

@lukasbestle Yes please, this would clarify a lot when digging deeper 😅

texnixe commented 5 months ago

Guess this needs an issue in the kirby repo to change the doc blocks accordingly, then as a next step change our logic to show advanced methods.

lukasbestle commented 5 months ago

What do you think @distantnative? If you agree on the concept, could you work on the implementation as well? I think you have the best overview of our docs reflection logic.

distantnative commented 5 months ago

I'm fine with switching to @internal and @advanced. 👍