Render Integer display width directly after the type - #185
Open
simon-mundy wants to merge 3 commits into
Open
Conversation
The "length" option was concatenated onto the specification string after Column::getExpressionData() had already appended NOT NULL and DEFAULT, so the display width landed after the column attributes and MySQL rejected the statement with error 1064. It was also the only column attribute to reach SQL by string concatenation rather than as an Argument. Insert "(%s)" directly after the type and splice the width in as a Literal, matching AbstractLengthColumn. The option accepts an int or a string of digits and anything else throws InvalidArgumentException, so no raw option value reaches the SQL. setOption() now accepts int, as the constructor options array and the documentation already did. Fixes php-db#178 Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
simon-mundy
added
bug
CI installs the newest Mago, and 1.47.5 changed how the analyzer handles nested array shapes: prepareDataHierarchy() re-binds a reference into the MetadataData shape on every loop iteration, which no longer completes in useful time. Re-type the reference as a plain array so the shape is not re-derived per iteration, and adjust the expected findings accordingly. Regenerate the analyzer baseline for the reworded findings in Select, PredicateSet, Profiler and AbstractSql (the issues are unchanged, the messages are not), and reduce the continuation indent of two multi-line conditions in AbstractTableGateway to match the 1.47.4+ formatter. Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
The tests declare their covered units with CoversMethod, so the private helper added to Integer was executed but credited to nothing, and codecov reported the patch as 40% covered. Signed-off-by: Simon Mundy <simon.mundy@peptolab.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
1 participant
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.
Integer::getExpressionData()now inserts(%s)directly after the type and splices the display width in at index 2 as aLiteral, sonew Integer('i', false, null, ['length' => 11])renders"i" INTEGER(11) NOT NULLand a nullable column with a default rendersBIGINT(20) NULL DEFAULT '7'.BigIntegerandSmallIntegerinherit it. Nothing underIntegerconcatenates an option value into the spec any more.The
lengthoption accepts anintor a string of digits; anything else throwsSql\Exception\InvalidArgumentException, since the value goes out as a rawLiteral.Column::setOption()is widened tobool|int|stringsosetOption('length', 11)works understrict_types, as the constructor$optionsarray andcolumns.mdalready allowed. Overlaps #169.columns.mdnotes that MySQL deprecated integer display width in 8.0.17 and that the MySQL decorator will drop the attribute (phpdb-mysql#81); other platforms keep rendering it.Same render-time approach as #183, implemented separately so the two PRs don't depend on each other. Worth hoisting into a shared hook on
Columnonce both are in.IntegerTestasserts the full rendered string rather than containment,setOption('length', 11)is covered inColumnTestand through rendering, invalid option values are covered by a data provider, andBigIntegerTest/SmallIntegerTesteach get a rendering test.Fixes #178