Skip to content

Render Integer display width directly after the type - #185

Merged
simon-mundy merged 5 commits into
php-db:0.6.xfrom
simon-mundy:fix/178-integer-display-width
Sep 28, 2026
Merged

Render Integer display width directly after the type#185
simon-mundy merged 5 commits into
php-db:0.6.xfrom
simon-mundy:fix/178-integer-display-width

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

Integer::getExpressionData() now inserts (%s) directly after the type and splices the display width in at index 2 as a Literal, so new Integer('i', false, null, ['length' => 11]) renders "i" INTEGER(11) NOT NULL and a nullable column with a default renders BIGINT(20) NULL DEFAULT '7'. BigInteger and SmallInteger inherit it. Nothing under Integer concatenates an option value into the spec any more.

The length option accepts an int or a string of digits; anything else throws Sql\Exception\InvalidArgumentException, since the value goes out as a raw Literal. Column::setOption() is widened to bool|int|string so setOption('length', 11) works under strict_types, as the constructor $options array and columns.md already allowed. Overlaps #169.

columns.md notes 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 Column once both are in.

IntegerTest asserts the full rendered string rather than containment, setOption('length', 11) is covered in ColumnTest and through rendering, invalid option values are covered by a data provider, and BigIntegerTest/SmallIntegerTest each get a rendering test.

Fixes #178

@simon-mundy
Render Integer display width directly after the type
b42f80f 
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

codecov Bot commented Sep 7, 2026 •
edited
Loading

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 self-assigned this Sep 7, 2026
@github-project-automation github-project-automation Bot moved this to Todo in @phpdb Sep 7, 2026
@simon-mundy simon-mundy added this to the 0.6.0 milestone 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
Keep the QA toolchain green under Mago 1.47.6
e3a2a62 
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
Attribute normaliseDisplayWidth() coverage to IntegerTest
6107c1b 
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>
@simon-mundy
simon-mundy requested a review from tyrsson September 7, 2026 11:35
@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
@simon-mundy
simon-mundy merged commit 69c8da3 into php-db:0.6.x Sep 28, 2026
15 checks passed
@simon-mundy simon-mundy mentioned this pull request Sep 28, 2026
Merged
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>
simon-mundy added a commit that referenced this pull request Sep 29, 2026
@simon-mundy
Close 68 escaped mutants across metadata, SQL and table gateway
cd2827b 
Escaped mutants drop from 79 to 11 and Covered MSI rises from 97% to 99%.

The largest cluster was one root cause: AbstractSourceTest seeds the data
property by reflection before calling a getter, so the getter's own loader
call is unobservable and Infection deletes it for free. LazyRecordingSource
seeds only inside loadXData(), delegates to its parent (which keeps the
parent's protected visibility observable from a subclass) and records each
call, so dropping a loader call from a getter now empties the result. That
plus two genuine gaps in the same file - constraint keys matching on only
half the table/constraint pair, and the hierarchy builder auto-vivifying
nulls - accounts for 33 of them.

Elsewhere: Values and Identifiers had no test file at all; the predicates
round-tripped setSpecification() without checking getExpressionData()
honoured it; the length columns never covered a value long enough for the
array_splice insert to displace anything; and Integer's display width, new
in #185, never saw a zero width or a values array long enough to notice a
removal.

Twelve mutants are verified equivalents rather than gaps and are ignored in
infection.json5.dist with reasons: the six loader guards short-circuit an
idempotent prepareDataHierarchy(), getColumns() delegates to getColumnNames()
which reloads the same data, implode() discards the keys array_fill() sets,
a RowGatewayInterface is caught by the next guard anyway, and
stopPropagation() has an empty body.

The 11 remaining escapes are all exception-message concatenation. Killing
them means pinning exact wording, which buys brittleness rather than safety,
so they are left visible rather than suppressed.
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.

Integer length option renders the display width after NOT NULL

2 participants

@simon-mundy @tyrsson