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

Add support for DateTime() #891

Closed
benmajor opened this issue Mar 16, 2022 · 22 comments
Closed

Add support for DateTime() #891

benmajor opened this issue Mar 16, 2022 · 22 comments

Comments

@benmajor
Copy link
Contributor

This is more a topic of discussion than a bug, but I thought it may be worth mentioning. It would be super-handy if RedBean was aware of DateTime assignments to bean properties. I know we can achieve the same functionality by using format(), or implement it using models, but the following would be extremely convenient:

$user =  R::dispense('user');
$user->firstName = 'John';
$user->lastName = 'Doe';
$user->created = new \DateTime();

R::store($user);

Of course in the above instance, we could easily switch it out for R::isoDateTime(), but if we're doing some sort of complex stuff with intervals, the above would be a nice feature IMO.

@gabordemooij
Copy link
Owner

Can you give an example of a more complex situation involving intervals?

@benmajor
Copy link
Contributor Author

Sure! Let's say that we want to calculate the user's trial end date:

$now = new DateTime();

$user = R::dispense('user');
$user->registered = $now; 
$user->trialEnd = $now->add(new DateInterval('P7D'));

R::store($user);

Considering the above, if RedBean had support for DateTime objects, we've essentially eliminated the need for two separate calls to the format() method.

@gabordemooij
Copy link
Owner

Okay, so all you want is for de setter to recognize this object? But what about loading? You don't want RedBeanPHP to turn every text that looks like a date into a DateTimeObject... Or is it not that useful for loading?

@benmajor
Copy link
Contributor Author

Well, adding support for the setter would be incredibly useful too, but that would represent a significant breaking change unless it was implemented using a flag or similar.

@gabordemooij
Copy link
Owner

I think modifying the setter can be implemented in a backward compatible manner. Modifying the getter requires a little thought. Maybe something like $date = $user->box('DateTime')->trialEnd or $user->getDateTime()->trialEnd ?

@benmajor
Copy link
Contributor Author

Yes, I think modifying the setter shouldn't cause any issues with maintaining backwards compatibility.

Regarding the getter, I currently implement this using __call() in an abstract entity as follows:

// Is it a valid datetime?
if (is_string($value) && \DateTime::createFromFormat('Y-m-d H:i:s', $value) !== false) {
    return \DateTime::createFromFormat('Y-m-d H:i:s', $value);
}

// Is it a valid date?
if (is_string($value) && \DateTime::createFromFormat('Y-m-d', $value) !== false) {
    return \DateTime::createFromFormat('Y-m-d', $value);
}

I think the latter syntax you've suggested for the getter is probably cleaner and more obvious to newer RedBean users, but I wonder if it may be better to use something like asDateTime() (i.e. $user->asDateTime()->trialEnd). Doing so would open up scope for additional types in the future if deemed appropriate (such as asJson(), for instance). Of course the latter is out of scope of this discussion, but a handy reserve to have, I think.

@gabordemooij
Copy link
Owner

Sounds good, I do some research and try some solutions

@benmajor
Copy link
Contributor Author

Thanks! I am happy to assist in any way possible.

@gabordemooij
Copy link
Owner

I was just about the implement this new feature today when I discovered it already seems to be partially implemented:

image

@gabordemooij
Copy link
Owner

At least the setter should already be working in the current versions of RedBeanPHP, there is even a unit test for it:

image

@benmajor
Copy link
Contributor Author

benmajor commented Apr 2, 2022

Okay that's great. I have to admit that I didn't try it, as I was just using an abstract model to do it. Adding support for getters would be great though.

@gabordemooij
Copy link
Owner

Would this be a solution?

@benmajor
Copy link
Contributor Author

benmajor commented Apr 2, 2022

I think that's a perfect solution!

@benmajor
Copy link
Contributor Author

benmajor commented Apr 4, 2022

Thanks for your work on this @gabordemooij , I think this is a great addition to the library. I really like the solution for fetching a bean's value and casting it to an object of a specific type too, that's really neat.

@benmajor benmajor closed this as completed Apr 4, 2022
@benmajor
Copy link
Contributor Author

benmajor commented Feb 8, 2023

Sorry to re-open this, but I wonder if the type check referenced here (

} elseif ( $value instanceof \DateTime ) { $value = $value->format( 'Y-m-d H:i:s' ); }
) should actually be:

$value instanceof \DateTimeInterface

Just so that we have support for additional PHP Date instances, such as DateTimeImmutable?

@benmajor benmajor reopened this Feb 8, 2023
@Lynesth
Copy link
Collaborator

Lynesth commented Feb 8, 2023

While I agree with you, DateTimeInterface seems to have been added in 5.5.
This means we'd need a polyfill since Redbean supports 5.3+.

@benmajor
Copy link
Contributor Author

benmajor commented Feb 9, 2023

Hmm, is there a particular reason for the support of 5.3? Given that security patch support ended 8 years ago, surely it must be considered EOL by now?

@Lynesth
Copy link
Collaborator

Lynesth commented Feb 9, 2023

There's one reason called Gabor :D

@marios88
Copy link
Contributor

marios88 commented Feb 9, 2023

@benmajor Personally when i need functionality like this use custom getters and setters in the model

    public function setLogintime(CarbonImmutable $logintime){
        $this->bean->logintime = $logintime->format(MYSQL_DATETIME_FORMAT);
    }
    
    public function getLogintime(): CarbonImmutable {
        return CarbonImmutable::createFromFormat(MYSQL_DATETIME_FORMAT, $this->bean->logintime);
    }

@benmajor
Copy link
Contributor Author

@marios88 yes, this functionality can be implemented via custom getters and setters. Indeed, that's how I've implemented such functionality to date. But given that there's a core test for \DateTime, it seems logical to me that there should be similar support for \DateTimeImmutable and similar. That having been said, I do appreciate this may introduce additional complications, given the required for support of = 5.3!

@gabordemooij
Copy link
Owner

I will look into this.

@gabordemooij
Copy link
Owner

@Lynesth @marios88 @benmajor maybe I'm wrong but it seems that instanceof just does not care if the actual class exist, so we can just change this without sacrificing any backward compatibility I think...

I know my obsession with backward compatibility is a bit over the top, but on the other hand, if we can fix it without breaking bc, that's always nice. The internal RedBeanPHP code may be a bit "hairy" because of it but it's not like you have to work inside the library code all the time.

gabordemooij added a commit that referenced this issue Feb 18, 2023
Add check DateTimeInterface, Closes #891.
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