Conversation
jooola
force-pushed
the
ci-tests
branch
2 times, most recently
from
September 19, 2026 17:57
20e020d to
7cec6b7
Compare
Travis dropped support for the free OSS plan this project relied on, and the Docker-based local/CI runner (docker/*) targeted a MySQL service container. Replaced both with a GitHub Actions workflow that runs the suite against PostgreSQL (matching the fixture switch in the next commit) across PHP 8.2 and 8.4, plus a slimmed-down Dockerfile/Makefile/ compose.yml for reproducing the same setup locally via 'make dev' / 'make reset test'. Also bumps composer.json's minimum PHP version to 7.4 (the lowest version actually exercised by the new CI matrix; the old '>7.1' constraint predates it) and relaxes the phing/phing constraint from ~2.4 (2.4.* only) to ^2.4 (2.4.x or later 2.x).
CI now runs against a PostgreSQL service container instead of MySQL, so the bookstore/namespaced/schemas fixtures' build.properties and runtime-conf.xml switch their adapter/DSN/credentials accordingly. Two fixture schemas needed an explicit <unique> added: PostgreSQL (unlike MySQL) requires a foreign key's referenced columns to be covered by a unique constraint. record_label's 'id' column (bookstore/schema.xml) and bookstore_contest's 'bookstore_id'+'id' pair (schemas/schema.xml) are referenced by other tables' foreign keys but weren't previously covered by one. reset_tests.sh now special-cases the 'bookstore' fixture to rebuild last: 'namespaced' targets the same physical tables under a different PHP namespace, and its DDL DROPs them; PostgreSQL requires that DROP to CASCADE (there's no MySQL-style FOREIGN_KEY_CHECKS=0 to suppress it), which also drops other tables' (media, review, ...) foreign keys onto the dropped ones. Rebuilding bookstore last leaves its constraints standing. It also skips the mysql/ reverse-engineering fixture, which needs a live MySQL server that's no longer part of the test infrastructure - MysqlSchemaParserTest is skipped to match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… MySQL Several formatter tests build raw SQL by hand with e.g. book.TITLE = "foo" - MySQL's non-ANSI mode tolerates double quotes as a string literal delimiter, but standard SQL (and PostgreSQL) treats "foo" as a quoted *identifier*, not a string. Against PostgreSQL this became "column \"foo\" does not exist" instead of a real query. Switched them to single-quoted string literals, which is valid SQL everywhere and was clearly the intent (there's no column, table, or alias named foo/Quicksilver/etc. anywhere in these queries). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…r returns one
incrementQueryCount() was declared ': int' but its body only ever did
$this->queryCount++; with no return statement. PHP throws a TypeError
at runtime ('must return a value of type int, none returned') the
moment execution reaches the end of a non-void-typed function without
returning - which happens on every call, since nothing here ever
returned a value even before the type was added. Nothing reads this
method's return value (checked every call site), so declare it void
instead of inventing a return value nothing needs.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…erator/runtime Passing null where a string is expected is deprecated since PHP 8.1, and was flooding test output: - XMLElement::booleanValue() -> strtolower(null) - by far the loudest, triggered on every parsed XML attribute default value (~25k lines in a full test run). - Database::addTable() -> strpos(null, ...) on a table's namespace. - PropelColumnComparator::compareColumns() -> strtoupper(null) on a column's SQL type. - sfYamlInline::dumpScalar() -> ctype_digit(int) (PHP 8.1 also deprecated ctype_*'s legacy int-as-ASCII-code coercion). Also declares two properties that were being set dynamically without ever being declared on the class (deprecated since PHP 8.2): - XmlToAppData::$firstPass (write-only, dead code - never read anywhere, so just declaring it is enough). - Table::$isArchiveTable, via #[AllowDynamicProperties] rather than a real declared property, since ArchivableBehavior relies on property_exists() to distinguish archive tables from regular ones - declaring the property on the class would make it "exist" on every Table and break that check. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
DelegateBehavior::objectCall() decides whether to forward an unknown
method call to a delegate object with:
is_callable(array('$ARFQCN', $name))
is_callable() with a plain class-name string (no instance) used to be
lenient about non-static instance methods, matching PHP's old
allowance for calling them statically. PHP 8 made that a hard error,
and is_callable() now correctly reports these methods as not callable
that way - so it never returns true for a real, public, non-static
setter/getter, and every delegated call falls through to
BaseObject::__call(), which throws "Call to undefined method".
This broke the entire delegate behavior under PHP 8 (DelegateBehaviorTest
consistently failed with PropelException: Call to undefined method:
setSubtitle - the first delegated setter it exercises). Switched the
check to method_exists(), which reports plain existence regardless of
calling convention and is what this check actually means to ask.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A full `vendor/bin/phpunit` run produced ~28,000 deprecation lines
(96 distinct message+location combinations). All are now fixed; a
clean run now produces zero (the only deprecation left anywhere in
the pipeline is from vendor/phing, third-party code we don't own and
composer would just overwrite).
The fixes fall into a few groups:
- "Passing null to a non-nullable string parameter" (strtolower,
strtoupper, strpos, strrpos, strspn, strtok, trim, str_replace,
preg_match, parse_url, file_exists, iconv, DOMDocument::createTextNode)
across generator/lib and runtime/lib, plus the code templates that
generate query filters (QueryBuilder) and slugs (SluggableBehavior) -
fixed with (string) casts or explicit null guards at each call site.
- pg_escape_string() in PgsqlPlatform::disconnectedEscapeText() requires
a live connection since PHP 8.1, defeating the point of a
*disconnected* escape - switched to the generic quote-doubling
fallback used when there's no connection.
- Dynamic properties (deprecated since PHP 8.2) on real classes that
never declared them: several *BuilderModifier classes' $queryClassname
cache, ConcreteInheritance{,Parent}Behavior's $builder cache,
ForeignKey's $isParentChild flag, and a missing $queryClassname
declaration in the many-to-many collection template
(PHP5ObjectBuilder) that its one-to-many counterpart already had.
DebugPDO gets #[AllowDynamicProperties] instead of a real property,
since the one dynamic property it's given (by a test hook) is
test-only, not part of its real API.
- Same dynamic-property issue in test files - declared the missing
properties, except: ObjectBehaviorTest had two copy-paste typos
($this->preDelete/postDelete instead of $t->...) that happened to
only manifest as this deprecation because the real target already
gets a real property below; TableMapTest::$foo was dead test-only
scaffolding, so just removed.
- Table3's dozen or so hook flags (preSave, postDelete, etc.), set
dynamically by the test-only TestAllHooksBehavior via injected code
strings - declared them all through its own objectAttributes() hook
instead of leaving them dynamic.
- #[ReturnTypeWillChange] on methods whose return type became
incompatible with their SPL/PDO parent interface once PHP added
native return types there (DebugPDOStatement, NestedSetRecursiveIterator,
PropelConfigurationIterator, and the nested-set getIterator()
template - there turned out to be two separate templates for it).
- ReflectionParameter::getClass() (removed in PHP 8, not just
deprecated in the log because the fallback path happened to still
work) replaced with getType()->getName().
- "Optional parameter before a required one" in ModelCriteria(WithNamespace/Schema)Test's
test helper signatures - the default value was never actually used
by any call site, so just dropped it.
Verified with a full clean run before and after: identical
Tests: 2663, Errors: 429, Failures: 121, Warnings: 1, Skipped: 16 -
only the deprecation noise changed.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
assertClassHasAttribute() is deprecated in PHPUnit 9 and slated for removal in PHPUnit 10, and PHPUnit's own deprecation handling was counting the call as a warning on every run. property_exists() is the direct replacement for what this test actually checks (that the generated class declares the attribute, regardless of visibility or whether an instance has been constructed). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…yntax generator/default.properties' propel.defaultDateFormat (%x) and propel.defaultTimeFormat (%X) were never updated when strftime-style formatting was removed from the generated accessors (see the 'remove deprecated strftime date format support' change, which only updated propel.defaultTimeStampFormat). The generated getter for every DATE/TIME column defaults its $format parameter to whichever of these properties applies, and explicitly throws "strftime format not supported anymore" whenever $format contains '%' - so simply calling getBar1() with no arguments on any DATE or TIME column unconditionally threw. Replaced both with DateTime::format()-compatible equivalents matching the style already used for TIMESTAMP (Y-m-d H:i:s). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… values $boundValue could be a resource for BLOB/CLOB columns bound as PDO::PARAM_LOB, but getExecutedQueryString() only special-cased strings and null before passing $boundValue straight to str_replace() as the replacement value - which requires array|string and throws a TypeError on a resource. Every save() of an object with a BLOB/CLOB column hit this the moment debug logging tried to render the executed query (e.g. any test touching Media's cover_image/excerpt). Represent LOB values as the placeholder text already used elsewhere in this same class for debug logging, instead of the raw resource. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ry later test Issue617Test's setUp() swaps Propel's global configuration to point at a live MySQL server (via PlatformDatabaseBuildTimeBase) to test MysqlSchemaParser directly. There's no MySQL server in this environment, so that connection attempt throws before $this->con is ever assigned. Issue617Test::tearDown() unconditionally called $this->con->query(...) to clean up its test tables, which threw "call to a member function query() on null" - a second failure that occurred *before* it ever reached parent::tearDown(), which is what restores the original (pre-swap) Propel configuration. The net effect: any time this test's setUp() failed, the swapped-out MySQL-only configuration - which has no "bookstore" datasource at all - stayed in place as Propel's global state for the rest of the entire test run. Every other test needing the "bookstore" datasource then failed too, cascading into roughly 300 unrelated errors from a single root cause. Guard removeTables() against a connection that was never established, so tearDown() always reaches parent::tearDown() and the configuration gets restored regardless of how setUp() failed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A nested PropelPDO::rollBack() only sets $isUncommitable - it never issues a real ROLLBACK at that depth. Under PostgreSQL (unlike MySQL) that leaves the underlying transaction open and aborted, so every further statement on the same pooled connection fails until a real ROLLBACK happens. Any test whose setUp()/body errors out partway through a nested transaction (or a raw-SQL debug-query test that errors deliberately) was leaving this broken state for every later test that reused the connection - PropelPDOTest's own tests and PropelArrayFormatterTest::testFormatNoCriteria were both failing this way for reasons unrelated to what they actually test. Force the connection/transaction closed in tearDown() for the affected test bases (BookstoreTestBase, CmsTestBase) and PropelPDOTest itself, so a failure in one test can't poison the rest of the suite. Also guards BookstoreTestBase's pre-existing commit() against a nested transaction that was opened but never matched by a commit()/rollBack() at all. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.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.
No description provided.