alleyinteractive / alleyinteractive/mantle-framework

Factory::make() collides with plain strings that match global functions

Open
#915 0 comments 0 reactions 0 assignees View on GitHub
php
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.