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

New Reflectable package for reference docs #2415

Open
wants to merge 28 commits into
base: release/v5
Choose a base branch
from

Conversation

distantnative
Copy link
Member

@distantnative distantnative commented Mar 1, 2025

Description

Summary of changes

  • New Kirby\Reference\Reflectable package
    • Moves reflection logic into an object-oriented structure with dedicated classes (Returns, Deprecated etc.)
  • New Kirby\Reference\Types package
    • Restructures type-logic in a much cleaner way with a types collection and individual type objects
  • Support for type templates/generics
    • Not all cases covered but most (e.g. trait generics currently not functional)
  • @internal marked are now still included in the reference, but a warning is displayed on the detail page

Reasoning

Our code in the page models and one types class was getting overwhelmingly complex. Copping it up in dedicated classes makes it a lot easier to work with.


Todos

Bugs

Optional enhancements

@distantnative distantnative self-assigned this Mar 1, 2025
@lukasbestle
Copy link
Member

As I wrote on Discord, I really like the new structure and modularity.

Quick note before I forget: This could also enable #2264 more easily, right? Could well be a different PR though as we need to update the tags in the core code anyway before it will be visible in the reference.

@distantnative
Copy link
Member Author

Yes, I even tried to add it quickly but then ran into a huge loading time. But that was because I wanted to filter them already in the ::children() methods and that mean building a Pages collection to directly run ->toArray() on it again which probably triggers all reflections etc. and is just the worst performance-wise. It should be ok when we just filter the output.

But yes, probably better as a second step when we replace most @internal annotations with @advanced - otherwise they'll be removed from the docs if we already introduce that change.

@lukasbestle
Copy link
Member

In production we could probably use a collection cache for this. I think with filtering we need it anyway, otherwise the overview pages will be incredibly slow.

@lukasbestle
Copy link
Member

lukasbestle commented Mar 4, 2025

A bug I noticed in the rewrite: In reference-classes.txt we have a menu field that defines the order of objects. With this PR, the separator lines between users/block and layouts/language suddenly get rendered as section headings on the reference overview page.

Edit: Not a new bug, it already happens on getkirby.com

@lukasbestle
Copy link
Member

lukasbestle commented Mar 5, 2025

A fatal error I found on /docs/reference/panel/fields/date and many other field and section pages:

Error thrown with message "Cannot instantiate abstract class Kirby\Reference\Reflectable\ReflectableFunction"

Stacktrace:
#22 Error in /var/www/site/plugins/site/src/Reference/Reflectable/ReflectableOptions.php:40
#21 Kirby\Reference\Reflectable\ReflectableOptions:factory in /var/www/site/plugins/site/extensions/tags.php:214
#20 Kirby\Filesystem\F:{closure} in /var/www/kirby/src/Text/KirbyTag.php:232
#19 Kirby\Text\KirbyTag:render in /var/www/kirby/src/Text/KirbyTags.php:43
#18 Kirby\Text\KirbyTags:Kirby\Text\{closure} in [internal]:0
#17 preg_replace_callback in /var/www/kirby/src/Text/KirbyTags.php:39
#16 Kirby\Text\KirbyTags:parse in /var/www/kirby/src/Cms/App.php:885
#15 Kirby\Cms\App:kirbytags in /var/www/kirby/src/Cms/App.php:899
#14 Kirby\Cms\App:kirbytext in /var/www/kirby/config/methods.php:403
#13 Kirby\Cms\Core:{closure} in /var/www/kirby/src/Content/Field.php:69
#12 Kirby\Content\Field:__call in /var/www/site/templates/reference-article.php:4
#11 include in /var/www/kirby/src/Filesystem/F.php:434
#10 Kirby\Filesystem\F:loadIsolated in /var/www/kirby/src/Filesystem/F.php:374
#9 Kirby\Filesystem\F:Kirby\Filesystem\{closure} in /var/www/kirby/src/Filesystem/F.php:380
#8 Kirby\Filesystem\F:load in /var/www/kirby/src/Toolkit/Tpl.php:35
#7 Kirby\Toolkit\Tpl:load in /var/www/kirby/src/Template/Template.php:164
#6 Kirby\Template\Template:render in /var/www/kirby/src/Cms/Page.php:988
#5 Kirby\Cms\Page:Kirby\Cms\{closure} in /var/www/kirby/src/Content/VersionId.php:118
#4 Kirby\Content\VersionId:render in /var/www/kirby/src/Cms/Page.php:966
#3 Kirby\Cms\Page:render in /var/www/kirby/src/Cms/App.php:803
#2 Kirby\Cms\App:io in /var/www/kirby/src/Cms/App.php:1218
#1 Kirby\Cms\App:render in /var/www/index.php:7
#0 require in /Applications/Herd.app/Contents/Resources/valet/server.php:167

@lukasbestle
Copy link
Member

All blocks are marked as deprecated, looks kind of broken (from /docs/reference/panel/blocks/code):

Bildschirmfoto 2025-03-05 um 21 49 50

@lukasbestle
Copy link
Member

lukasbestle commented Mar 5, 2025

Several section headlines on /docs/reference/objects are missing and use the folder name (e.g. cms, database, toolkit).

Bildschirmfoto 2025-03-05 um 21 52 23

@lukasbestle
Copy link
Member

All $kirby methods use $kirby in the titles but $app in the method signatures. Not sure if intended:

Bildschirmfoto 2025-03-05 um 21 58 28

@distantnative
Copy link
Member Author

Several section headlines on /docs/reference/objects are missing and use the folder name (e.g. cms, database, toolkit).

That's the bug in the v5 beta not allowing for virtual content when a content folder foes exist.

@lukasbestle
Copy link
Member

I've started to collect the issues in the first post to keep an overview of what is already fixed.

@lukasbestle
Copy link
Member

Camel casing of class variable names is lost by converting to lowercase (e.g. /docs/reference/objects/cms/app-plugins/is-native-component):

Bildschirmfoto 2025-03-05 um 22 12 04

@distantnative
Copy link
Member Author

distantnative commented Mar 5, 2025

This lost camel-casing is not new. Maybe we can split the todos in regressions and nice-to-have enhancements. (I just saw you already have that, so maybe just moving that to the second section).

@lukasbestle
Copy link
Member

Oh, didn't realize that, normally I check but here I forgot. Will create a separate issue.

@distantnative distantnative linked an issue Mar 5, 2025 that may be closed by this pull request
@distantnative distantnative linked an issue Mar 5, 2025 that may be closed by this pull request
@distantnative distantnative linked an issue Mar 5, 2025 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants