alleyinteractive / alleyinteractive/mantle-framework
Factory::make() collides with plain strings that match global functions
- Dominant language
- PHP
- Stars
- 28
- Forks
- 7
- Avg merge
- 2d 4h
- Merged PRs (30d)
- 2
Description
### Description of the bug
`Factory::make()` decides whether an attribute value should be treated as a
lazy generator by checking `is_callable($value)`:
https://github.com/alleyinteractive/mantle-framework/blob/main/src/mantle/database/factory/class-factory.php#L360-L364
(mantle-framework/database v1.22.1, same location)
```php
foreach ( $args as $key => $value ) {
if ( is_callable( $value ) ) {
$args[ $key ] = $value( $key, $args );
}
}
```
`is_callable()` returns `true` for plain strings that name a real PHP function — not just for `Closure`s. PHP function names are also case-insensitive. So a perfectly ordinary literal string value can be silently invoked instead of used as-is, whenever it happens to match a global function name (a PHP builtin, or one of Mantle's own global helpers like `response()`, `view()`, `route()`, `redirect()`, `app()`, etc. defined in `mantle-framework/http`).
### Steps To Reproduce
```php
// 'Count' matches the PHP builtin count() case-insensitively.
$this->factory()->user->create_and_get( [
'first_name' => 'Count',
'last_name' => 'Dracula',
] );
// TypeError: count(): Argument #2 ($mode) must be of type int, array given
// -- $value('first_name', $args) becomes count('first_name', $args).
```
```php
// 'response' matches Mantle's own global response() helper.
$this->factory()->term->create_and_get( [
'taxonomy' => 'response',
'name' => 'Some Term',
] );
// Error: Call to undefined function app()
// -- response('taxonomy', $args) calls app(Response_Factory::class)
// internally, which isn't available outside a booted app container.
```
Both are ordinary, meaningful fixture values (a person's first name, a taxonomy slug) with no relation to callables — the failures are confusing and far removed from the actual cause, since the stack trace points into `Factory.php`/`Pipeline.php` rather than to the fixture data itself.
## Why I think this is worth narrowing
I checked every `definition()` method shipped in `mantle-framework/database` (`Post_Factory`, `Term_Factory`, `User_Factory`, `Comment_Factory`, `Attachment_Factory`, `Blog_Factory`, `Network_Factory`, `Fluent_Factory`) and none of them ever supply a non-`Closure` callable (no array-callables, no bare function-name strings) as an attribute value — every value is eagerly computed (e.g. `$this->faker->sentence()`). I couldn't find any call site, in this project or elsewhere I could check, that relies on `is_callable()`'s broader matching (string function names, array callables, invokable objects) versus a plain `instanceof \Closure` check.
The `$value( $key, $args )` invocation shape also reads as "a closure that wants to know its own key and the sibling args," which is exactly the lazy-attribute pattern `instanceof \Closure` would still support.
### Additional Information
```php
foreach ( $args as $key => $value ) {
if ( $value instanceof \Closure ) {
$args[ $key ] = $value( $key, $args );
}
}
```
If there's a real use case for supporting array-callables or invokable objects as lazy attribute values that I'm missing, happy to hear it — but as-is, this silently miscategorizes ordinary string/array data any time it coincides with a defined function name, which seems more likely to bite people than to be relied upon.
Contributor guide
Assessment
This issue has not been assessed yet.