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

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

Closed
eerison opened this issue Dec 1, 2023 · 3 comments
Closed

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

eerison opened this issue Dec 1, 2023 · 3 comments
Labels

Comments

@eerison
Copy link
Contributor

eerison commented Dec 1, 2023

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:
- name
- slug
- title (maybe ?)

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.

@eerison eerison added the feature label Dec 1, 2023
@GeraudBourdin
Copy link
Contributor

GeraudBourdin commented Dec 3, 2023

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
Copy link
Contributor Author

eerison commented Dec 3, 2023

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
Copy link
Member

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.

  1. Removing those ? are BC break.

So I think it can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants