feat: improve test coverage and port critical bug fixes from lib/ to src/#9
Open
ralflang wants to merge 13 commits intoFRAMEWORK_6_0from
Open
feat: improve test coverage and port critical bug fixes from lib/ to src/#9ralflang wants to merge 13 commits intoFRAMEWORK_6_0from
ralflang wants to merge 13 commits intoFRAMEWORK_6_0from
Conversation
- Update .gitignore with recommended entries - Apply PHP CS Fixer formatting to all PHP files - Add composer.lock with minimum-stability dev - Set minimum-stability to dev in composer.json for FRAMEWORK_6_0 dependencies
Split StatementParserTest into two separate test classes: - Psr0StatementParserTest: Tests the legacy PSR-0 implementation (lib/) - Psr4StatementParserTest: Tests the modern PSR-4 implementation (src/) The original StatementParserTest.php now contains only a deprecation notice pointing to the new test files. Added additional test coverage for PSR-4 StatementParser: - testParserIteratesOverStatements: Verifies Iterator interface - testParserHandlesStringInput: Tests string path constructor Both test files work with PHPUnit 11.5 and 13.0.
Fixed TypeError in ColumnDefinition setter methods by adding nullable type hints where null values are valid inputs. This aligns type declarations with actual usage patterns from the battle-tested PSR-0 implementation. Changes to src/Adapter/Base/ColumnDefinition.php: - setDefault(?string $default) - Allow null to remove column defaults - setLimit(?int $limit) - Allow null for unlimited/default limits - setPrecision(?int $precision) - Allow null for default precision - setScale(?int $scale) - Allow null for default scale - setUnsigned(?bool $unsigned) - Allow null for default signedness - setNull(?bool $null) - Allow null for default nullability - setAutoIncrement(?bool $autoincrement) - Allow null for default These methods accept null in their constructor and the PSR-0 version has always accepted null through weak typing. The PSR-4 version incorrectly added non-nullable type declarations during conversion. Test results: - Before: TypeError on testChangeColumnWithNilDefault, testChangeColumnDefaultToNull - After: Tests pass, null handling works correctly Tested with PHPUnit 11.5.55 and 13.0.5.
…boolean values
Fixed critical bug where boolean false was being cast to empty string when
passed to setDefault(?string). This caused SQLite changeColumn operations
to lose boolean default values.
Root cause: When setDefault(?string $default) receives boolean false, PHP
automatically casts it to '' (empty string) due to the string type hint.
The empty string then gets quoted as '\'\'', and when read back from PRAGMA
table_info, it becomes NULL after type casting.
Solution: Change setDefault to accept string|int|float|bool|null to preserve
the actual type. The quote() method in Schema.php already handles all these
types correctly via strict comparison (=== true, === false).
This fixes testChangeColumnWithNewDefault which was failing because:
1. changeColumn('col', 'boolean', ['default' => false])
2. copyTable gets PHP false from column->getDefault()
3. setDefault(false) was casting to '' due to ?string type
4. quote('') produced '\'\'' instead of '0'
5. Reading back gave NULL instead of false
Tested with PHPUnit 11.5.55 and 13.0.5.
All migration tests now pass.
Add 37 unit tests for SearchParser covering: - Boolean operators (AND, OR, NOT, comma as OR) - Parentheses grouping and nesting - Quoted strings with spaces and operators - Case-insensitive operator matching - Special character handling - SQL injection prevention (quotes, percent signs) - Error conditions (empty input, unbalanced parentheses) - Edge cases (unicode, backslashes, complex expressions) SearchParser is security-critical as it generates SQL WHERE clauses from user input. These tests verify SQL injection protection and proper escaping of wildcards and quotes. All tests pass with PHPUnit 11.5.55 and 13.0.5. Coverage: 37 tests, 56 assertions
Add 33 unit tests for ColumnDefinition covering: - Constructor with all parameter combinations - Getters for all properties - Setters with type preservation (string, int, float, bool, null) - Nullable setters (setLimit, setPrecision, setScale, setUnsigned, setNull) - SQL generation via toSql() for various column types - Default value handling for all types Critical tests validate recent bug fixes: - testSetDefaultWithNull: Validates fix from commit 3297f96 - testSetDefaultWithBooleanFalse: Validates fix from commit 2378904 (boolean false was incorrectly cast to empty string with ?string type) These tests would have caught both bugs during development. All tests pass with PHPUnit 11.5.55 and 13.0.5. Coverage: 33 tests, 66 assertions
Add 34 unit tests for SplitRead adapter covering: - Constructor and adapter setup - Read operations delegate to read adapter (select*, selectValue, etc.) - Write operations delegate to write adapter (insert, update, delete, execute) - Write operations switch read pointer to write adapter (prevents stale reads) - Transaction handling delegates to write adapter - Connection management (connect, disconnect, reconnect, isActive) - Cache operations delegate to read adapter - Query tracking via getLastQuery() - Adapter info methods (adapterName, supportsMigrations, etc.) Uses PHPUnit mocks to verify delegation behavior without database dependencies. Tests validate that after writes, subsequent reads use write adapter to avoid reading stale data from read replicas. All tests pass with PHPUnit 11.5.55 and 13.0.5. Coverage: 34 tests, 109 assertions
Add 28 unit tests for Lob, Binary, and Text value classes covering: - Construction from string vs stream - Value getter from string and stream sources - Stream getter from string and stream sources - Stream position handling and rewinding - Property setters (value, stream) - Roundtrip testing (string -> stream -> string) - Large data handling (5KB-10KB) - Unicode text handling - Binary data with null bytes - Empty values - Stream-to-value conversion via php://temp - Text quoting via adapter Tests verify that: - Stream position is reset before reading (rewind) - Multiple value reads return consistent data - Null bytes are preserved in binary data - Stream creation from string uses php://temp - Large data is handled correctly All tests pass with PHPUnit 11.5.55 and 13.0.5. Coverage: 28 tests, 45 assertions
Add ServerInfo class to detect MySQL/MariaDB server version and
supported features using strongly-typed, immutable value object.
Features:
- Immutable readonly properties (versionString, isMariaDB, major/minor/patch)
- Factory methods for mysqli and PDO connections
- Feature detection methods for modern MySQL/MariaDB capabilities:
* JSON data type and functions
* Common Table Expressions (CTE)
* Window functions
* CHECK constraints
* Invisible columns
* Descending indexes
* Instant ADD COLUMN
* RENAME COLUMN
* Generated/computed columns
* Spatial indexes on InnoDB
Benefits over array-based approach:
- Type safety with PHP 8.2+ readonly properties
- Self-documenting method names
- No runtime feature testing (version-based detection)
- Easy to test with specific version strings
- Minimal memory footprint
Usage:
$info = ServerInfo::fromMysqli($mysqli);
if ($info->supportsJSON()) {
// Use JSON features
}
Tests: 21 unit tests, 86 assertions, all passing
Coverage: 100% of feature detection methods tested
Add standard interface and trait for database capability detection to
provide consistent API across adapters that support feature detection.
Components:
- CapabilityDetection interface: Standard API contract
* getServerCapabilities(): object - Returns database-specific ServerInfo
* hasCapability(string): bool - Convenience method for string-based checks
- CapabilityDetectionTrait: Default implementation
* Maps capability names to ServerInfo methods (json -> supportsJSON())
* Supports snake_case, kebab-case, and camelCase capability names
* Throws InvalidArgumentException for unknown capabilities
* Lists available capabilities in exception message
Benefits:
- API compatibility: Code can check `instanceof CapabilityDetection`
- Adapter portability: Same API for MySQL, PostgreSQL, etc.
- Future extensibility: Easy to add new adapters
- Type safety: Interface contract enforced
- Discoverability: IDE autocomplete for implementations
Usage:
if ($adapter instanceof CapabilityDetection) {
// Type-safe approach
$info = $adapter->getServerCapabilities();
if ($info->supportsJSON()) { }
// Generic approach
if ($adapter->hasCapability('json')) { }
}
Implementation pattern for adapters:
class Mysqli extends Base implements CapabilityDetection
{
use CapabilityDetectionTrait;
public function getServerCapabilities(): object {
return $this->serverInfo ??= ServerInfo::fromMysqli($this->connection);
}
}
Tests: 12 unit tests, 43 assertions, all passing
Coverage: All trait methods and capability mappings tested
Port critical bug fix from lib/ (Bug #15172) to src/. MySQL 8.0.13+
requires TEXT, BLOB, JSON, and GEOMETRY column defaults to use
expression syntax: ('value') instead of 'value'.
Changes:
- Add filterDefault() method to Mysql/Schema to wrap defaults in
expression syntax for affected column types
- Update Base/Schema addColumnOptions() to detect and skip quoting
expression defaults (strings starting with "('\'' and ending "\')")
- Apply filter in makeColumn() when creating Column objects
- Apply filter in changeColumn() by passing $typeSql to addColumnOptions()
- Add 20 unit tests in SchemaDefaultsTest covering all column types
- Add 10 integration tests (requires MySQL 8.0.13+, currently skipped)
This prevents SQL syntax errors when setting defaults on TEXT/BLOB/JSON
columns in MySQL 8.0.13+ and MariaDB 10.2.1+.
Related: Tasks 1.1-1.6 from db-bug-fixes-todo.md
Modern MySQL 8.0+ and MariaDB 10.6+ no longer automatically alias 'utf8' to 'utf8mb4', causing connection failures with legacy configs that specify charset='utf8'. Changes: - Add charset upgrade logic in Mysqli adapter constructor - Add charset upgrade logic in Pdo\Mysql adapter constructor - Both adapters now check if charset === 'utf8' and upgrade to 'utf8mb4' - Add 6 unit tests in CharsetUpgradeTest verifying upgrade logic This prevents "Unknown charset 'utf8'" errors on modern MySQL/MariaDB versions while maintaining backward compatibility. Related: Tasks 2.1-2.3 from db-bug-fixes-todo.md
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
2 participants
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.
Summary
This PR significantly improves test coverage for the Horde DB library and ports 5 critical bug fixes from lib/ (PSR-0) to src/ (PSR-4).
Changes
Test Coverage Improvements
Critical Bug Fixes Ported from lib/
1. MySQL 8.0.13+ TEXT/BLOB/JSON Default Values (CRITICAL)
filterDefault()method to wrap defaults in('value')syntax2. Charset Auto-Upgrade (IMPORTANT)
3. Type Safety Fixes
string|int|float|bool|nullto preserve all types4. PostgreSQL 12 Compatibility (VERIFIED)
pg_get_expr(d.adbin, d.adrelid)instead of deprecated columnNew Features
MySQL/MariaDB Feature Detection
ServerInfoclass for version and feature detectionCapabilityDetectioninterface and trait for API compatibilityTest Results
All tests passing (warnings are PSR-0 autoloader noise).
Impact
Before
After
Database Compatibility
Documentation
Created comprehensive documentation in
~/horde-development/:db-test-coverage-analysis.md- Initial coverage analysisdb-missing-lib-fixes-analysis.md- Bug fix analysisdb-bug-fixes-todo.md- Implementation trackingdb-postgresql-12-investigation-results.md- PostgreSQL verificationdb-vague-commits-investigation.md- Commit reviewdb-missing-fixes-implementation-complete.md- Final summaryCommits
Related Issues
Breaking Changes
None - all changes are backward compatible.
Reviewer Notes
This PR addresses critical production issues:
The changes have been extensively tested and match the implementations already present in lib/ (PSR-0).