Fix model table resolution from Source attributes by QUAD69 · Pull Request #17742 · phalcon/cphalcon

QUAD69 · 2026-11-07T17:13:24Z

Hello!

Type: bug fix
Link to issue: [BUG]: ORM ignores model Source attributes and annotations #17741
Related: [NFR]: Attributes for Annotations #17601, CP-17601 annotation attributes #17603

In raising this pull request, I confirm the following:

I have read and understood the Contributing Guidelines
I have checked that another pull request for this purpose does not exist
I wrote some tests for this PR
I have updated the relevant CHANGELOG
I have created a documentation PR about this change

#[Source(table: 'co_customers')] is read by AttributesReader, but the ORM ignores it and uses customers for a Customers model. This causes queries to target the wrong table unless the model also calls setSource().

Resolve the class-level Source attribute only when the container's annotations service explicitly uses AttributesReader (or a subclass). A private getDefaultModelSource() method checks the service interface and that opt-in, then delegates table extraction to Source::getTable(Collection). Both positional and named table arguments are supported. Explicit setSource() / setModelSource() values retain priority, the existing per-class source cache stores the result, and the manager applies its current table prefix on each lookup.

An invalid annotations service raises InvalidAnnotationsService, including when the service returns a non-object value. A Source attribute with a missing, empty, or non-string table raises InvalidSourceTable; a collection without Source returns null so the manager keeps the inferred-name fallback. An explicit source bypasses attribute lookup and validation.

The default PHPDoc reader is not invoked for source resolution, so existing Introspection models keep working even when their unrelated docblocks are rejected by the annotation parser. PHPDoc @Source remains ignored. Missing DI/annotations services and classes without Source retain the inferred-name fallback; foreign attributes named Source do not override it.

Attributes are not inherited: a subclass without its own Source gets the table inferred from the subclass name. An inherited initialize() that calls setSource() still explicitly sets the child's source. Unit and SQLite tests cover both cases, an AttributesReader subclass, attribute-based persistence with both metadata strategies, and an Introspection model with malformed docblocks. Table resolution retains the configured metadata strategy. The Source API docblock includes a usage example; applications still configure AttributesReader on their annotations adapter.

Validation on PHP 8.5.10, Zephir 1.7.0 and PHPUnit 10.5.66:

Model Manager Source unit suite: 55 tests, 110 assertions, all passing.
Source::getTable unit suite: 15 tests, 25 assertions, all passing. The 70 targeted unit tests and 7 SQLite tests also pass in randomized order with --fail-on-warning.
Source SQLite integration suite: 7 tests, 46 assertions, all passing.
Related unit tests: 814 tests, 1900 assertions, all passing.
SQLite Manager, metadata-strategy and source tests: 99 tests, 352 assertions, 1 skipped, no failures.
Additional SQLite model tests excluding cache services and metadata-adapter tests: 743 tests, 2129 assertions, 11 skipped, no failures. These selected runs overlap; the counts are not additive.
PHPCS, PHP-CS-Fixer and PHPStan level 1 pass for the new and changed Source test files and fixtures; git diff --check passes.
The CI MassMock smoke test passes: 1 test, 1187 assertions.
Generated IDE stubs pass Psalm. The local container lacks the Memcached extension, so this check explicitly loads Psalm's bundled Memcached stub via a temporary config kept outside the PR.

Regression tests cover arbitrary adapters implementing AdapterInterface, invalid service types, retry after replacing an invalid shared service, and explicit-source priority over malformed attributes. The scalar-service cases reproduced five PHP warnings on the previous guard before adding the object-type check.

The full suites requiring Redis/Memcached services, MySQL/PostgreSQL, and the remaining PHP-version matrix were not run locally.

Thanks!

Resolve a class-level Source through the configured annotations adapter before falling back to the uncamelized model name. Support positional and named table arguments while preserving explicit sources and table prefixes. Add regression coverage for both readers, optional DI services, per-class caching, column metadata, and SQLite persistence and queries. Assisted-by: Codex

Assisted-by: Codex

niden

Very nice fix and thank you for this. The logic i s correct but you missed the point where this starts running for every single model.

Looking forward to your fixes!

Only consult class attributes when the annotations service uses AttributesReader. Extract fallback resolution to a private method so the default docblock reader and Introspection models retain their behavior. Cover malformed docblocks, unannotated subclasses, and inherited explicit sources. Align test names and author/since tags with the repository style, clarify the Source API documentation, and shorten the changelog entry. Assisted-by: Codex

Exercise attribute-based persistence and PHQL with Introspection as well as Annotations, and assert that source resolution retains the configured metadata strategy. Cover the opt-in behavior of an AttributesReader subclass. Assisted-by: Codex

QUAD69 · 2026-11-08T12:55:05Z

Thanks for the review, @niden! All six comments have been addressed:

Source resolution is now opt-in through AttributesReader (including subclasses), with the lookup and fallback logic extracted into getDefaultModelSource().
The default PHPDoc reader is no longer parsed for table resolution. Regression tests cover Introspection models with malformed docblocks.
The Source API docblock and PR description now consistently describe attribute-only support; PHPDoc @Source remains ignored.
Added inheritance tests: a subclass without its own Source gets its inferred table name, while an inherited initialize() calling setSource() still sets the explicit source.
Shortened the changelog and moved the details to the companion documentation PR: Document opt-in model table resolution from Source attributes documentation#351.
Updated test names and added @author / @since annotations to match the project conventions.

Additional tests cover both metadata strategies and an AttributesReader subclass. The selected local unit/SQLite suites and style/static-analysis checks pass; the documentation build and lint also pass.

Ready for another review. Thank you!

niden

Thank you for the quick reply!

Second pass, mostly defensive checks and one coupling refactoring. This should be ready after that.

niden · 2026-11-10T20:25:42Z

+    {
+        parent::setUp();
+
+        $this->container = new FactoryDefault();


Just to be on the safe side, can you add a Di::reset() before this line. It will shield us from any other test before this, being left without its DI reset.

niden · 2026-11-10T20:29:03Z

+                let table = sourceAnnotation->getArgument(0);
+            }
+
+            if typeof table === "string" {


Add a check for when the table is also empty. A Source attribute with no name will make the table an empty array. Even better, throw an exception if the table name is empty.

Move table extraction and validation into Source::getTable(). Reject invalid annotations services and missing, empty or non-string Source table names with dedicated exceptions. Guard non-object services before Zephir's instanceof check to avoid warnings. Preserve AttributesReader opt-in, explicit-source precedence and fallback behavior. Cover custom adapters, invalid values, retry after failure and malformed-attribute overrides, reset DI during test setup, and keep private methods in alphabetical order. Assisted-by: Codex

Explain that missing, empty and non-string Source table names raise InvalidSourceTable during source resolution, matching phalcon/cphalcon#17742. Assisted-by: Codex

QUAD69 · 2026-11-13T13:32:15Z

Thanks for the second review, @niden! All five follow-up comments are addressed:

Validate the annotations service against AdapterInterface and throw a dedicated exception for invalid services. Non-object values are checked before instanceof to avoid Zephir warnings.
Move table extraction and validation into Source::getTable(Collection), with an exception for missing, empty, or non-string table names.
Reset DI before unit-test setup and put the private helper in alphabetical order.

Regression tests also cover custom adapters, explicit-source precedence over malformed attributes, and retry after an invalid service. All 77 targeted unit/SQLite tests pass in randomized order with --fail-on-warning; the related suites and style/static-analysis checks pass too.

The companion documentation is updated in phalcon/documentation#351, now in the 5.24 guides. Ready for another review. Thank you!

niden

Round 3. Thank you for all the fixes and the quick turnaround. I think the first issue is the most pressing because it is a potential BC change, the rest are just tidying up.

niden · 2026-11-14T01:21:57Z

+
+        let annotations = <AnnotationsAdapterInterface> container->getShared("annotations");
+
+        if typeof annotations !== "object" || !(annotations instanceof AnnotationsAdapterInterface) {


I think we have an issue here.

Some apps register a service that is not a Phalcon adapter as annotations, for example a Doctrine reader. In these apps, each model without setSource() now throws InvalidAnnotationsService. Before the PR, these apps worked with the default Introspection strategy. Only the annotations router and the Annotations metadata strategy read this service, and only when the app selects them. Now the manager reads it for every model. This conflicts with the opt-in rule from round 1.

I did ask for the throw and I should have thought about this issue beforehand so apologies. I think the best way to avoid BC changes and get what we need is to return the inferred source when the service is not an AdapterInterface (it counts as "not opted in"). Then we can remove the InvalidAnnotationsService.

niden · 2026-11-14T01:24:24Z

+            throw new InvalidAnnotationsService();
+        }
+
+        if !(annotations->getReader() instanceof AttributesReader) {


It seems like we are fixing one thing another comes up. With this, now the Manager knows about the opt-in rule i.e. the AttributesReader which increases coupling again.

We can move the reader check and the lookup into Source, say Source::getTableFromAdapter(<AdapterInterface> adapter, string className) -> string | null. The manager will keep only the DI lookup and will know nothing other than Source.

niden · 2026-11-14T01:27:32Z

+            return source;
+        }
+
+        let classAnnotations = annotations->get(get_class(model))->getClassAnnotations();


Performance related here: Reflection.

There is no real winner here if we want to introduce this - the cost though has to be highlighted. Some apps use AttributesReader with Introspection, for example apps that use attributes only for routing. In these apps, get() now reads the attributes of the class, its constants, its properties and all its methods, including the inherited Phalcon\Mvc\Model methods. With the Memory adapter, this occurs once per model class on each request. Before the PR, this cost was zero. With the Annotations strategy there is no extra cost, because that strategy already reads the same cached data.

We need to make a change in the documentation to advise developers to use a persistent adapter such as Apcu or Stream when using the AttributesReader. Memory carries the reflection cost.

niden · 2026-11-14T01:36:27Z

+        }
+
+        if typeof table !== "string" || table === "" {
+            throw new InvalidSourceTable();


This exception travels upwards. As such it will not be caught if my mechanism as a developer is to catch the mvc\model\exception (most common case). Wrap the code in the manager with a try catch and re-throw it as model\exception, using this exception in $previous.

niden · 2026-11-14T01:37:28Z

+            let table = sourceAnnotation->getArgument(0);
+        }
+
+        if typeof table !== "string" || table === "" {


Can we use trim(table) === "" to shield from a table name with only spaces? Rare, very edge, agreed but it costs very little to shield from it.

niden · 2026-11-14T01:38:00Z

+{
+    public function __construct()
+    {
+        parent::__construct("Source table name must be a non-empty string");


Can we inject the $table here as a parameter and use it in the messge? A bit more info.

QUAD69 added 2 commits November 7, 2026 20:07

docs: link Source table resolution fix to issue phalcon#17741

260921d

Assisted-by: Codex

niden requested changes Nov 7, 2026

View reviewed changes

QUAD69 changed the title ~~Fix model table resolution from Source attributes and annotations~~ Fix model table resolution from Source attributes Nov 8, 2026

QUAD69 mentioned this pull request Nov 8, 2026

Document opt-in model table resolution from Source attributes phalcon/documentation#351

Open

QUAD69 requested a review from niden November 8, 2026 19:43

niden requested changes Nov 10, 2026

View reviewed changes

QUAD69 requested a review from niden November 13, 2026 13:55

niden requested changes Nov 14, 2026

View reviewed changes


		let annotations = <AnnotationsAdapterInterface> container->getShared("annotations");

		if typeof annotations !== "object" \|\| !(annotations instanceof AnnotationsAdapterInterface) {

Conversation

QUAD69 commented Nov 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

niden left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

QUAD69 commented Nov 8, 2026

Uh oh!

niden left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

QUAD69 commented Nov 13, 2026

Uh oh!

niden left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

QUAD69 commented Nov 7, 2026 •

edited

Loading