search

silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
720 stars 820 forks source link

NEW Allow DataObject classes to define scaffolded relation formfields #11269

Closed GuySartorelli closed 3 weeks ago

GuySartorelli commented 3 weeks ago

Implements new API for scaffolding has_one, has_many, and many_many relations.

Needs https://github.com/silverstripe/silverstripe-assets/pull/613 to ensure file and image has_one relations are scaffolded with the same logic they were before, hence the bump in dependency.

Issue

GuySartorelli commented 3 weeks ago

Noticed a small issue - if $includeInOwnTab was false, it would still create the tab. Now it doesn't create the tab unless that is set to true. Also added test coverage to check that.

GuySartorelli commented 3 weeks ago

These new API are not called when the DataObject is a SiteTree, because that of course does its own thing. Worth mentioning that in the docs.

I think it's known that SiteTree doesn't use scaffolded form fields. I'm not sure where you'd want that documented. Feel free to point out somewhere specific to add it if you want it added (i.e. just the changelog? Or in the scaffolding page generally somewhere?) - and please make that comment in the docs PR instead of here.

GuySartorelli commented 3 weeks ago

Seems that $includeInOwnTab doesn't work for many_many, though does work for has_many. I did update the parent gridfield like in the doc instructions, so may be related to that. I do have the latest sha https://github.com/silverstripe/silverstripe-framework/commit/cd7225850033f7018a6e481c81a2c55277d5e535 for framework

You're setting it, but then you're calling the parent method which sets it again. You need to set it after calling the parent method.

If you like I can remove the bit that's setting it in the parent method, since I think it's setting it to the same as the default value anyway?

GuySartorelli commented 3 weeks ago

There's no consideration given for extension hooks. We should probably add these for each of the new methods. For instance the first thing I tried was to swap out LinkField for something else on Link, though I'm unable to do that.

I had intentionally excluded them because this is going to be called a lot and that means the overhead of calling extensions isn't going to be small.

I can add them if you still want them added after knowing that context.

emteknetnz commented 3 weeks ago

I've made an recommended up to the docs PR regarding SiteTree forms

Tested moving the $includeInOwnTab locally, you are correct it worth when setting afterwards

May as well keep the hooks off, yes there would be a lot of extra hook calls, and it's a pretty niche scenario. People still have the option of swapping out something like core classes with their own subclass using Injector