Don’t pass kirby as property to models

[bastianallgeier]

This PR …

With the refactoring of property setters in models in v4, Kirby got passed as an instance to propertyData['kirby'] This broke serializing and could have other side effects. I wonder if some whoops issues are related to it as well.

I've tracked down all the parts where we pass Kirby to a model. This should have no side-effects in theory. The models use static::$kirby as property and don't fetch the Kirby instance from the propertyData array anyway. But it remains a bit risk.

Fixes

• Objects are serializable again [K4] Session: need to serialize data. Regression? #6061

Breaking changes

• Hopefully none

Ready?

• Unit tests for fixed bug/feature
• In-code documentation (wherever needed)
• Tests and checks all pass

For review team

• Add changes to release notes draft in Notion
• Add to website docs release checklist (if needed)

[lukasbestle]

Passing the app object as property indeed seems superfluous as it isn't used anywhere at the moment. I can't find any reason why this would break anything.

The question is what our future plans with object coupling are. I think in general it would be desirable to avoid using global objects wherever possible, e.g. when using Kirby on the CLI with multiple independent app objects. But that's definitely a question for the future, we can still add the app objects as props back in later.

What we could also consider in the future is to implement custom serialization logic with the Serializable interface. It really doesn't make much sense to serialize all property data with every object.

[afbora]

Not sure but I think also you need to check out following lines: (check serialize($kirby->user()) to test it)

• https://github.com/getkirby/kirby/blob/fix/6061-serializable-objects/src/Cms/AppUsers.php#L90
• https://github.com/getkirby/kirby/blob/fix/6061-serializable-objects/src/Cms/AppUsers.php#L131

[bastianallgeier]

@afbora I found two more places where we pass kirby to the user models.

[afbora]

@bastianallgeier Sorry, I just pointing what I found similar ones. Is this possible one?
https://github.com/getkirby/kirby/blob/fix/6061-serializable-objects/src/Cms/App.php#L1470

[bastianallgeier]

@afbora yep, we don't even use it in the role class

[lukasbestle]

Those changes all look safe to me as I can't find any place in our code that tries to access those kirby props. Also can't find more places where we pass but don't use the kirby prop. A few classes (Cms\Api and Cms\Auth\Status) actually rely on it, but those are commonly not serialized.