sonata-project / SonataPageBundle

This bundle provides a Site and Page management through container and block services
https://docs.sonata-project.org/projects/SonataPageBundle
MIT License
216 stars 209 forks source link

[Model/Page] Remove nullable return for methods that is required #1731

Closed eerison closed 9 months ago

eerison commented 9 months ago

I have a suggestion (and may want to implement it 🙂) I can't do that atm :(

But I guess it is important we have this issue open to fix models, that contains nullable fields, But it shouldn't.

Context

Into the Models we have defined some nullable methods for example Model/Page::getName(): ?string, and we should return just string.

Page:

Note: I am adding just page here, because I want to avoid huge issue that gonna spend a lot of time, when we fix one, we can go to other model.

GeraudBourdin commented 9 months ago

hello, be aware that _page_internal_global have null slug/url/title. In my app, most of the rows have null title.

By the way i don't really get the point on removing the null return ? In a database pov, i prefer storing null value and not empty string to avoid a return type error

eerison commented 9 months ago

hello, be aware that _page_internal_global have null slug/url/title. In my app, most of the rows have null title.

By the way i don't really get the point on removing the null return ? In a database pov, i prefer storing null value and not empty string to avoid a return type error

Hmmm interesting, But don't you need those fields to have pages working 🤔🤔.

If you don't have those fields, how do you use those pages 🤔

VincentLanglet commented 9 months ago

I have a suggestion (and may want to implement it 🙂) I can't do that atm :(

But I guess it is important we have this issue open to fix models, that contains nullable fields, But it shouldn't.

Context

Into the Models we have defined some nullable methods for example Model/Page::getName(): ?string, and we should return just string.

1) When you write new Page(), no title/slug/name are set, so you require nullable getter or

$page = new Page();
$page->getName();

will crash. Those getters are used by Sonata when you're creating a page.

2) Removing those ? are BC break.

So I think it can be closed.