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

Coerce DBBoolean to be actually be a boolean #11332

Closed
4 tasks done
maxime-rainville opened this issue Aug 12, 2024 · 4 comments
Closed
4 tasks done

Coerce DBBoolean to be actually be a boolean #11332

maxime-rainville opened this issue Aug 12, 2024 · 4 comments

Comments

@maxime-rainville
Copy link
Contributor

Description

DBBoolean doesn't actually force its value to be a boolean. In fact, it seems to really want its value to be an int(1) or int(0). This seems like an anathema as we boldly step into our glorious strongly typed future.

I'm proposing that values should be coerced to be actual boolean when managed via DBBoolean.

I'm assuming we'll consider this a breaking change, so it will probably have to be in CMS 6 ... although I could imagine a straight face argument that the current implementation doesn't make any promise about whether it's going to return a bool or an int, so you probably shouldn't make an assumption either way.

Additional context or points of discussion

This is the status quo. Let's say you have a page like this.

class Page extends SiteTree
{
    private static $db = [
        'Truthiness' => 'Boolean',
    ];

    private static $has_one = [];
}

... and this task.

class Task extends BuildTask
{

    public function run($request)
    {
        $page = Page::create();
        $page->Truthiness = true;
        $page->Title = 'I am true';
        var_dump($page->Truthiness);
        $page->write();

        $page = Page::get()->byID($page->ID);
        var_dump($page->Truthiness);

        $page = Page::create();
        $page->Truthiness = false;
        $page->Title = 'I am false';
        var_dump($page->Truthiness);
        $page->write();

        $page = Page::get()->byID($page->ID);
        var_dump($page->Truthiness);
    }

}

You will get the following output:

bool(true)
int(1)
bool(false)
int(0)

Validations

  • You intend to implement the feature yourself
  • You have read the contributing guide
  • You strongly believe this feature should be in core, rather than being its own community module
  • You have checked for existing issues or pull requests related to this feature (and didn't find any)
@maxime-rainville maxime-rainville changed the title Coerces DBBoolean to be actually be a boolean Coerce DBBoolean to be actually be a boolean Aug 12, 2024
@lekoala
Copy link
Contributor

lekoala commented Aug 13, 2024

for my part i don't mind so much the mixed int|bool type as long as it's clearly stated
see silverleague/silverstripe-ideannotator#166
nothing prevents adding a getBool() method if you really need typed booleans
having ints kind of make sense since this is how it is actually stored in the db

@GuySartorelli
Copy link
Member

GuySartorelli commented Aug 13, 2024

You'll find a lot of the DBField implementations don't force values to be a specific type. It's another area where strict typing hasn't made it's appearance yet.
For example, some areas of the core code actually rely on the fact that arrays can be cast as DBText, which holds the array as its value, and then can convert that to a JSON string via the JSON() helper method.

I don't think the way the data is stored in the database matters as much as it would have when these classes were originally created - despite the name starting with DB they are used just as much for the presentation layer now. Only the prepValueForDB(), nullValue(), and setValue() methods really need to handle ints for the database's sake, IMO.

I'd support coercing values into stricter types across DBField classes, but that would need to happen in a major release (as you noted in the issue description) and shouldn't be scoped to a single DBField subclass. It should be done across the board all in the same release.

In the meantime, PHP is very happy for you to pass ints around even where the typehint is for boolean - it's happy to coerce them into bool on your behalf, so I don't think there's any need even for a getBool() method.

@maxime-rainville
Copy link
Contributor Author

That makes a lot of sense. Let us coerce all data into submission!

@emteknetnz
Copy link
Member

Closing as work is being done as part of #11403

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

No branches or pull requests

4 participants