Conversation
In NO_ENGINE_SUBSTITUTION MySQL will happily accept empty strings for INT. But in a standard mode it will fail. Better to be explicit.
This riffs on SET/JSON behaviours in this PR: https://github.com/Karmabunny/sprout3/pull/160/changes Co-Authored-By: Benno Lang <benno@truevault.com.au>
Co-Authored-By: Benno Lang <benno@truevault.com.au>
From this PR: Karmabunny/sprout3#175 Co-Authored-By: Benno Lang <benno@truevault.com.au>
There was a problem hiding this comment.
Pull request overview
This PR consolidates model typecasting/default-handling by introducing a populate() pathway for building models from DB rows, adding round-trip support for native typed properties (incl. SET/JSON/datetime), and relocating/expanding tests around that behavior.
Changes:
- Add
PdbModelInterface::populate()and update result hydration (PdbReturn) to use it for Pdb models. - Implement type casting on hydrate and type-aware serialization on save (arrays→SET/JSON, bool→int, DateTime→string).
- Add a dedicated test table/model and PHPUnit coverage for typed save/load and defaults.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
tests/db_struct.xml |
Adds a new dummy table with JSON/SET and defaulted columns to exercise typed behavior. |
tests/PdbModelTest.php |
Adds save/load/default override tests using the new typed dummy model. |
tests/Models/TypedModel.php |
Introduces a typed test model and custom save data composition. |
src/PdbModelTrait.php |
Adds populate(), typecasting on hydrate, and serialization rules in getSaveData(). |
src/PdbModelInterface.php |
Adds the new populate() method to the public model interface. |
src/Pdb.php |
Ensures boolean parameters bind as integers in prepared statements. |
src/Models/PdbReturn.php |
Switches hydration to populate() when hydrating PdbModelInterface classes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Use DateTimeImmutable as the fallback. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This combines a few PRs from Sprout.
We somewhat had a split implementation between settings defaults and typecasting within Sprout. I also wanted the tests located closer to the implementation.
I've modified the original json/sets PR into a more generic typecasting helper. This also accommodates date field types and anything else someone might want to patch in.
Sprout also runs with
NO_ENGINE_SUBSTITUTEat all times. Which isn't ideal because it's a weaker model than the default SQL mode. In that I also found that booleans were also not casted correctly when committed.The piece that makes this all a bit more possible is the introduction of the
populate()method. PdbModelTrait doesn't (and can't) enforce the Configurable interface, and vaguely aligning the types didn't feel right either. More importantly this solves other problems that I came up against recently where I wanted to distinguish between data from the DB and data from a controller. Something that a singleupdate()couldn't easily solve.