Skip to content

Render length columns without a length instead of throwing ValueError - #183

Merged
simon-mundy merged 4 commits into
php-db:0.6.xfrom
simon-mundy:fix/176-length-column-null-length
Sep 28, 2026
Merged

Render length columns without a length instead of throwing ValueError#183
simon-mundy merged 4 commits into
php-db:0.6.xfrom
simon-mundy:fix/176-length-column-null-length

Conversation

@simon-mundy

@simon-mundy simon-mundy commented Sep 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Member
Q A
Documentation yes
Bugfix yes
BC Break no
New Feature no
RFC no
QA yes
House Keeping no

AbstractLengthColumn now builds its specification at render time. (%s) and the length Literal go 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, Floating and Double render bare (CHAR NOT NULL, DECIMAL NOT NULL). Varchar and Varbinary throw Sql\Exception\InvalidArgumentException naming the column — SQL-92 and MySQL both reject a bare VARCHAR, so the rule belongs in core rather than the platform. A decimal scale without digits also throws instead of rendering DECIMAL(,2). Text and Blob still ignore any configured length.

Length 0 is still treated as unset, as before. I went with the render-time exception rather than making $length required in the Varchar/Varbinary constructors: 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(), and VarcharTest::testGetExpressionDataWithNullLength asserts the exception instead of pinning the mismatch. getLengthExpression() is covered for both-null, digits-only and decimal-only. Existing tests are untouched.

Fixes #176

@simon-mundy
Render length columns without a length instead of throwing ValueError
7d1ea95 
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

codecov Bot commented Sep 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@simon-mundy simon-mundy mentioned this pull request Sep 7, 2026
Merged
@simon-mundy simon-mundy self-assigned this Sep 7, 2026
@simon-mundy simon-mundy added bug Something isn't working qa Improvements in quality assurance of the project labels Sep 7, 2026
@simon-mundy simon-mundy added this to the 0.6.0 milestone Sep 7, 2026
@github-project-automation github-project-automation Bot moved this to Todo in @phpdb Sep 7, 2026
@simon-mundy
Keep the QA toolchain green under Mago 1.47.6
1a35abf 
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>
@simon-mundy
simon-mundy requested a review from tyrsson September 7, 2026 11:36
@simon-mundy simon-mundy added the documentation Improvements or additions to documentation label Sep 7, 2026
tyrsson
tyrsson previously approved these changes Sep 28, 2026
View reviewed changes

@tyrsson tyrsson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved post conflict removal

@simon-mundy
simon-mundy merged commit fc0a1ed into php-db:0.6.x Sep 28, 2026
15 checks passed
@github-project-automation github-project-automation Bot moved this from In Progress to Done in @phpdb Sep 28, 2026
simon-mundy added a commit to simon-mundy/phpdb that referenced this pull request Sep 28, 2026
@simon-mundy
Route the Sql throw sites through named constructors
84e1538 
All 55 throw sites in src/Sql now call a named constructor. Message text is
byte-identical, verified template by template against the original literals.

- Add Sql\Predicate\Exception with its own ExceptionInterface,
  InvalidArgumentException and RuntimeException, holding the 10 templates the
  predicate classes use, so the messages sit with the classes that raise them
  rather than concentrating on one component catalogue
- Add 24 templates and factories to Sql\Exception\InvalidArgumentException and
  6 to Sql\Exception\RuntimeException, the last four covering the Ddl column
  and check constraint messages added by php-db#183, php-db#184 and php-db#185
- Collapse nine duplicated messages onto one template each, five of which the
  audit on php-db#189 had missed: the four "intended to work with only the table"
  copies in Sql, the two parameter-count copies in AbstractSql, the two column
  key copies in Insert, the two read-only copies in Select, and the subject
  messages shared by Platform and AbstractPlatform
- Fix a copy-paste bug in Platform::getSqlString(), which reported
  prepareStatement() as the method with no effect
- Retype Between's \LogicException and AbstractSql's \ValueError to the package
  types, so both reach the marker interface
- Declare the @throws Predicate::unnest() and Predicate::__get() never had
- Cover every factory with a template test, and derive the 46 Sql message
  assertions from the templates rather than restating them
- Re-attribute the two baselined sprintf argument-type entries to the factories
  that now receive those arguments, and baseline the three halstead readings the
  refactor shifts
- Exclude src/Sql/Exception/ from too-many-methods in mago.toml. The threshold
  is 10 and a catalogue holds one template per condition a component can
  report, so the count tracks failure modes rather than anything a split would
  improve

Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
This was referenced Sep 28, 2026
Merged
Closed
simon-mundy added a commit to simon-mundy/phpdb that referenced this pull request Sep 28, 2026
@simon-mundy
Route the Sql throw sites through named constructors
df142fd 
All 55 throw sites in src/Sql now call a named constructor. Message text is
byte-identical, verified template by template against the original literals.

- Add Sql\Predicate\Exception with its own ExceptionInterface,
  InvalidArgumentException and RuntimeException, holding the 10 templates the
  predicate classes use, so the messages sit with the classes that raise them
  rather than concentrating on one component catalogue, and register both
  concrete classes in ExceptionHierarchyTest's EXCEPTION_CLASSES map
- Add 24 templates and factories to Sql\Exception\InvalidArgumentException and
  6 to Sql\Exception\RuntimeException, the last four covering the Ddl column
  and check constraint messages added by php-db#183, php-db#184 and php-db#185
- Collapse nine duplicated messages onto one template each, five of which the
  audit on php-db#189 had missed: the four "intended to work with only the table"
  copies in Sql, the two parameter-count copies in AbstractSql, the two column
  key copies in Insert, the two read-only copies in Select, and the subject
  messages shared by Platform and AbstractPlatform
- Fix a copy-paste bug in Platform::getSqlString(), which reported
  prepareStatement() as the method with no effect
- Retype Between's \LogicException and AbstractSql's \ValueError to the package
  types, so both reach the marker interface
- Declare the @throws Predicate::unnest() and Predicate::__get() never had
- Cover every factory with a template test, and derive the 46 Sql message
  assertions from the templates rather than restating them
- Re-attribute the two baselined sprintf argument-type entries to the factories
  that now receive those arguments, and baseline the three halstead readings the
  refactor shifts
- Exclude src/Sql/Exception/ from too-many-methods in mago.toml. The threshold
  is 10 and a catalogue holds one template per condition a component can
  report, so the count tracks failure modes rather than anything a split would
  improve

Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
simon-mundy added a commit to simon-mundy/phpdb that referenced this pull request Sep 29, 2026
@simon-mundy
Route the Sql throw sites through named constructors
1948b1a 
All 55 throw sites in src/Sql now call a named constructor. Message text is
byte-identical, verified template by template against the original literals.

- Add Sql\Predicate\Exception with an InvalidArgumentException and a
  RuntimeException holding the 10 templates the predicate classes use, so the
  messages sit with the classes that raise them rather than concentrating on one
  component catalogue, and register both in ExceptionHierarchyTest's
  EXCEPTION_CLASSES map. The namespace deliberately has no ExceptionInterface of
  its own: both classes extend their Sql\Exception counterparts, so they are
  already caught by Sql\Exception\ExceptionInterface, and a narrower marker
  would only collapse the same InvalidArgument and Runtime split a second time
- Add 24 templates and factories to Sql\Exception\InvalidArgumentException and
  6 to Sql\Exception\RuntimeException, the last four covering the Ddl column
  and check constraint messages added by php-db#183, php-db#184 and php-db#185
- Collapse nine duplicated messages onto one template each, five of which the
  audit on php-db#189 had missed: the four "intended to work with only the table"
  copies in Sql, the two parameter-count copies in AbstractSql, the two column
  key copies in Insert, the two read-only copies in Select, and the subject
  messages shared by Platform and AbstractPlatform
- Fix a copy-paste bug in Platform::getSqlString(), which reported
  prepareStatement() as the method with no effect
- Retype Between's \LogicException and AbstractSql's \ValueError to the package
  types, so both reach the marker interface
- Declare the @throws Predicate::unnest() and Predicate::__get() never had
- Cover every factory with a template test, and derive the 46 Sql message
  assertions from the templates rather than restating them
- Re-attribute the two baselined sprintf argument-type entries to the factories
  that now receive those arguments, and baseline the three halstead readings the
  refactor shifts
- Exclude src/Sql/Exception/ from too-many-methods in mago.toml. The threshold
  is 10 and a catalogue holds one template per condition a component can
  report, so the count tracks failure modes rather than anything a split would
  improve

Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
simon-mundy added a commit to simon-mundy/phpdb that referenced this pull request Sep 29, 2026
@simon-mundy
Route the Sql throw sites through named constructors
a63d366 
All 55 throw sites in src/Sql now call a named constructor. Message text is
byte-identical, verified template by template against the original literals.

- Add Sql\Predicate\Exception with an InvalidArgumentException and a
  RuntimeException holding the 10 templates the predicate classes use, so the
  messages sit with the classes that raise them rather than concentrating on one
  component catalogue, and register both in ExceptionHierarchyTest's
  EXCEPTION_CLASSES map. Neither namespace declares an ExceptionInterface: the
  package has one marker, PhpDb\Exception\ExceptionInterface, and every class here
  implements it by inheritance
- Add 24 templates and factories to Sql\Exception\InvalidArgumentException and
  6 to Sql\Exception\RuntimeException, the last four covering the Ddl column
  and check constraint messages added by php-db#183, php-db#184 and php-db#185
- Collapse nine duplicated messages onto one template each, five of which the
  audit on php-db#189 had missed: the four "intended to work with only the table"
  copies in Sql, the two parameter-count copies in AbstractSql, the two column
  key copies in Insert, the two read-only copies in Select, and the subject
  messages shared by Platform and AbstractPlatform
- Fix a copy-paste bug in Platform::getSqlString(), which reported
  prepareStatement() as the method with no effect
- Retype Between's \LogicException and AbstractSql's \ValueError to the package
  types, so both reach the marker interface
- Declare the @throws Predicate::unnest() and Predicate::__get() never had
- Cover every factory with a template test, and derive the 46 Sql message
  assertions from the templates rather than restating them
- Re-attribute the two baselined sprintf argument-type entries to the factories
  that now receive those arguments, and baseline the three halstead readings the
  refactor shifts
- Exclude src/Sql/Exception/ from too-many-methods in mago.toml. The threshold
  is 10 and a catalogue holds one template per condition a component can
  report, so the count tracks failure modes rather than anything a split would
  improve

Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tyrsson tyrsson tyrsson left review comments

Assignees

@simon-mundy simon-mundy

Labels

bug Something isn't working documentation Improvements or additions to documentation qa Improvements in quality assurance of the project

Projects

@phpdb
Status: Done

Milestone

0.6.0

Development

Successfully merging this pull request may close these issues.

AbstractLengthColumn subclasses throw ValueError when rendered without a length

2 participants

@simon-mundy @tyrsson