Skip to content

Fix OperationResponseAssociation ignoring controller defaultTable#582

Merged
cnizzardini merged 6 commits intocnizzardini:masterfrom
jamisonbryant:fix/respect-default-table-in-association-builder
Apr 22, 2026
Merged

Fix OperationResponseAssociation ignoring controller defaultTable#582
cnizzardini merged 6 commits intocnizzardini:masterfrom
jamisonbryant:fix/respect-default-table-in-association-builder

Conversation

@jamisonbryant
Copy link
Copy Markdown
Contributor

@jamisonbryant jamisonbryant commented Apr 21, 2026

Summary

OperationResponseAssociation::build() ignores a controller's $defaultTable property when resolving the database table for associations. This causes a DatabaseException when a controller name doesn't match its actual database table (e.g., MyCoolFeatureController that manages data from the foobars_unrelated table via protected string $defaultTable = 'FoobarsUnrelated').

The fix resolves the table via the controller's fetchTable() method (which respects $defaultTable) before falling back to the controller name. This aligns with the pattern already used in ModelScanner::routeHasModel().

Major Changes

  • Added protected resolveTableAlias() method to OperationResponseAssociation that resolves the table alias from the controller's fetchTable(), falling back to the controller name. Extracted from build() to stay under the cyclomatic/NPath complexity thresholds and to provide a mockable seam for tests.
  • Removed deprecated ReflectionProperty::setAccessible() call in ExceptionResponse (no-op since PHP 8.1, deprecated in 8.5).

Minor Changes

  • Added CustomTableController test fixture with $defaultTable = 'Employees' to simulate a controller whose name doesn't match its table
  • Added test_default_table_property_is_respected test case
  • Removed stale // TODO comment from test setUp
  • Code style fix via phpcbf (unqualified Throwable import)

Test Plan

  • Existing tests pass (controllers following naming conventions return the same alias from fetchTable())
  • New test verifies that a controller with $defaultTable set to a different table correctly resolves that table in the association builder
  • No PHP 8.5 deprecation warnings from setAccessible()

@jamisonbryant jamisonbryant marked this pull request as draft April 21, 2026 15:48
When associations['table'] is not explicitly set,
OperationResponseAssociation::build() now resolves the table via
the controller's fetchTable() method, which respects $defaultTable,
before falling back to the controller name. This aligns with the
pattern already used in ModelScanner::routeHasModel().
@jamisonbryant jamisonbryant force-pushed the fix/respect-default-table-in-association-builder branch from ba2aeef to 64272f3 Compare April 21, 2026 15:53
ReflectionProperty::setAccessible() has no effect since PHP 8.1
and is deprecated in PHP 8.5.
Moves the controller fetchTable logic into a protected method,
reducing cyclomatic and NPath complexity in build() and making
the table resolution mockable in tests.
@jamisonbryant jamisonbryant marked this pull request as ready for review April 21, 2026 16:15
@jamisonbryant
Copy link
Copy Markdown
Contributor Author

jamisonbryant commented Apr 21, 2026

@cnizzardini can you take a look when you get a chance and lmk if you like this fix?

In our application code, we are able to get by with setting the 'table' key of the associations property on our #[OpenApiResponse] attribute on the action, however this fix would be nice-to-have so we don't have to remember to keep it updated.

Comment thread src/Lib/Operation/ExceptionResponse.php
Comment thread src/Lib/Operation/OperationResponseAssociation.php Outdated
@cnizzardini
Copy link
Copy Markdown
Owner

Just a few comments @jamisonbryant but this looks like a good change! Surprised this went unnoticed for so long, pretty sure this has been in there since day 1. I'll pull this down as soon as I can and do some quick testing.

@jamisonbryant
Copy link
Copy Markdown
Contributor Author

btw really like the phpmd integration, loved how it yelled at me when I crossed the complexity threshold ❤️

@cnizzardini cnizzardini merged commit 50557de into cnizzardini:master Apr 22, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants