Render length columns without a length instead of throwing ValueError - #183
Open
simon-mundy wants to merge 2 commits into
Open
Render length columns without a length instead of throwing ValueError#183simon-mundy wants to merge 2 commits into
simon-mundy wants to merge 2 commits into
Conversation
Every AbstractLengthColumn subclass accepts a null length, but the fixed '%s %s(%s)' specification left vsprintf() with more placeholders than values whenever no length was set. Build the specification at render time instead: the length is inserted directly after the type only when one is set. Char, Binary, Decimal, Floating and Double now render bare without a length. Varchar and Varbinary, which SQL-92 and MySQL both reject without a length, throw InvalidArgumentException naming the column. A decimal scale without digits also throws instead of rendering "(,2)". Text and Blob keep ignoring any configured length. Fixes php-db#176 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>
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.
AbstractLengthColumnnow builds its specification at render time.(%s)and the lengthLiteralgo in directly after the type only when a length is set, so the placeholder and value counts always agree.Without a length,
Char,Binary,Decimal,FloatingandDoublerender bare (CHAR NOT NULL,DECIMAL NOT NULL).VarcharandVarbinarythrowSql\Exception\InvalidArgumentExceptionnaming the column — SQL-92 and MySQL both reject a bareVARCHAR, so the rule belongs in core rather than the platform. A decimal scale without digits also throws instead of renderingDECIMAL(,2).TextandBlobstill ignore any configured length.Length
0is still treated as unset, as before. I went with the render-time exception rather than making$lengthrequired in theVarchar/Varbinaryconstructors:setLength(null)exists either way so the render check is needed regardless, and this avoids the BC break. The constructor change can go on top if wanted.Each affected type now has a no-length test that goes through
CreateTable::getSqlString(), andVarcharTest::testGetExpressionDataWithNullLengthasserts the exception instead of pinning the mismatch.getLengthExpression()is covered for both-null, digits-only and decimal-only. Existing tests are untouched.Fixes #176