Make TimestampableEntity getter type hints nullable

[villermen]

Signals nullability for return types of TimestampableEntity::getCreatedAt() and TimestampableEntity::getUpdatedAt(). They may actually return null when they are not yet persisted.

This helps static type checkers understand this trait better.

[codecov-commenter]

Codecov Report

Base: 80.16% // Head: 80.16% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (b54a15e) compared to base (d1988ae).
Patch has no changes to coverable lines.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2531      +/-   ##
============================================
- Coverage     80.16%   80.16%   -0.01%     
  Complexity     3142     3142              
============================================
  Files           158      158              
  Lines          7705     7708       +3     
============================================
+ Hits           6177     6179       +2     
- Misses         1528     1529       +1     

Impacted Files	Coverage Δ
src/Timestampable/Traits/TimestampableEntity.php	100.00% <ø> (ø)
src/Tree/Strategy/ORM/Nested.php	95.68% <0.00%> (-0.28%)	⬇️
src/Uploadable/UploadableListener.php	93.54% <0.00%> (-0.03%)	⬇️
...e/Entity/Repository/MaterializedPathRepository.php	98.05% <0.00%> (-0.02%)	⬇️
src/Tree/Strategy/ORM/Closure.php	97.18% <0.00%> (+0.05%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[villermen]

Would love for this to receive a response, so please don't auto close.

[phansys]

Hi @villermen! Thank you for your first contribution.
Please, be aware about the PR's labels. It currently requires a rebase.

[phansys]

If we are going to allow null as return type for these methods, I guess we should first update the hints for the properties that are being returned here.

[villermen]

@phansys Thanks for the review! I have rebased the PR against main, and applied nullable hints to all timestampable traits.

Side question: Is there anything against adding them as native return type? E.g., getCreatedAt(): ?\DateTime? I noticed the minimum required PHP version is 7.2 where this is supported, and it's not technically a BC break right?

[phansys]

Side question: Is there anything against adding them as native return type? E.g., getCreatedAt(): ?\DateTime?

The reason behind that decision is to avoid issues for users that are not respecting the type hints declared in our API. Because its nature, I guess changes like this will be part of the next major release.

[phansys]

Thank you @villermen!

[nvdbeek]

It is technically correct that these properties may be null when retrieved before being persisted. However in which situations would you actually call these methods before then? Is it not improper to call this method when they are nullable, wouldn't be better to throw exceptions in the situation when this data is not available yet. Making these return types nullables forces us to either throw exceptions or add assertions which are unnecessary in most cases.

[villermen]

@nvdbeek I created this PR to have the documentation match the behavior, mainly to correct static type check inconsistencies I was experiencing. I would be in favor of a non-null property too, but I don't think an exception will help prevent undesirable situations. null conveys better that the value has not yet been set and allows creating simple fallback logic with ??, ?-> and the likes.

Forcing initialization of the properties upon construction (not on get) so that they are always available may also prove difficult when using traits. I wouldn't know how to approach that, but if you have an idea there a PR is probably welcome =)