Skip to content

[12.x] Allow globally disabling Factory parent relationships via Factory::dontExpandRelationshipsByDefault() #56154

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jul 8, 2025

Conversation

cosmastech
Copy link
Contributor

@cosmastech cosmastech commented Jun 27, 2025

A continuation of #53450. This feature was added by @browner12 and it's been incredibly helpful for our team in speeding up tests.

That said, it requires us to write ->withoutParents() on each factory make, which has led to bugs. Namely, we don't add the trait to refresh the database and then data gets left over between tests, which breaks expectations in other tests... but we don't discover it for a month 😆 😭 and then it's debug city.

It would be great if in the test's setup, we can just indicate no factory should create a parent relationship.

public function test_has_one_editor_permission_returns_true(): void
{
+    UserPermissionFactory::dontExpandRelationshipsByDefault();
+
    $collection = new UserPermissionCollection([
        UserPermission::factory()
-            ->withoutParents()
            ->make([
                'type' => 'viewer',
                'company_id' => 2,
                'product_id' => 789,
            ]),
        UserPermission::factory()
-            ->withoutParents()
            ->make([
                'type' => 'editor',
                'company_id' => 2,
                'product_id' => 432,
            ]),
    ]);

    $result = $collection->hasEditorForCompany(2);

    $this->assertTrue($result);
}

Even better, I would just add this to the class's setUp() method.

@NickSdot
Copy link
Contributor

Would a descriptive method be more Laravel-y?

UserPermissionFactory::disableExpandedRelationships()

@DarkGhostHunter
Copy link
Contributor

DarkGhostHunter commented Jul 4, 2025

This gave me an idea. What if we could declare the methods to call when invoking a given Factory inside the test? Something like this:

#[UserFactory('withoutParents', 'someOtherThing')]
class TestSomething extends TestCase
{
    // ...
}

Then, instead of replicating all calls in each test, only call the method on top of the class and call it a day.

My guess is that the Test Case would put these methods on a static array that would be called each time the Factory is instantiated, and flushed once the test case ends.

@cosmastech
Copy link
Contributor Author

This gave me an idea. What if we could declare the methods to call when invoking a given Factory inside the test? Something like this:

#[UserFactory('withoutParents', 'someOtherThing')]
class TestSomething extends TestCase
{
    // ...
}

Then, instead of replicating all calls in each test, only call the method on top of the class and call it a day.

My guess is that the Test Case would put these methods on a static array that would be called each time the Factory is instantiated, and flushed once the test case ends.

That's an interesting idea, though to me it feels like a pretty limited use-case 🤷 What other factory methods do you think a user might set via this attribute? And would it require userland to create an attribute for each factory? Maybe #[FactorySettings(UserFactory::class, ['withoutParents'])] would be less code?

I would prefer to see a way to setup the database once for all tests in a test case. That to me seems like a much more common problem, which would improve test run time.

Take for instance all of my test cases inside of a single class require a user, an organization, a user permission for that organization, and a team. These have to be built for each test method, and then are torn down at the end. Would love if there was a way to build that data once for all tests, run each test so they can leverage those common values, and then remove it once we're done with all tests in the class.

This is a totally separate issue to the one I'm proposing here, but just thinking that this would get some serious mileage in the codebase I am working on. I don't fully understand how parallel testing runs via paratest, so maybe this is a non-starter based on that. 🤔

@cosmastech
Copy link
Contributor Author

Would a descriptive method be more Laravel-y?

UserPermissionFactory::disableExpandedRelationships()

You're right. I had that thought, but came across these issues:

  • What is a good name for it? It's called withoutParents as an instance method, so the static method should be something similar (defaultWithParents() maybe?) Naming it disableExpandedRelationships(), you need a way to re-enable it as well. So it would take a bool as an argument. Feels weird to make the negative case the only way to access it. 🤷
  • It would be used in tests mostly, so I figured that giving it just a public static property was probably decent enough DX.

I'll wait to see what Taylor's thoughts are on this. Happy to make whatever changes are necessary.

@taylorotwell
Copy link
Member

I would do something like dontExpandRelationshipsByDefault.

@taylorotwell taylorotwell marked this pull request as draft July 7, 2025 14:33
@cosmastech
Copy link
Contributor Author

cosmastech commented Jul 7, 2025

I would do something like dontExpandRelationshipsByDefault.

@taylorotwell do you mean a static function with that name?

@cosmastech cosmastech force-pushed the static-without-parents branch from bf839be to 4291fc6 Compare July 7, 2025 16:04
@cosmastech cosmastech changed the title [12.x] Allow globally disabling Factory parent relationships [12.x] Allow globally disabling Factory parent relationships via Factory:: dontExpandRelationshipsByDefault() Jul 7, 2025
@cosmastech cosmastech marked this pull request as ready for review July 7, 2025 16:05
@cosmastech cosmastech changed the title [12.x] Allow globally disabling Factory parent relationships via Factory:: dontExpandRelationshipsByDefault() [12.x] Allow globally disabling Factory parent relationships via Factory::dontExpandRelationshipsByDefault() Jul 7, 2025
@taylorotwell taylorotwell merged commit 9f71c7b into laravel:12.x Jul 8, 2025
60 checks passed
@cosmastech cosmastech deleted the static-without-parents branch July 9, 2025 17:11
mohammad-fouladgar pushed a commit to mohammad-fouladgar/framework that referenced this pull request Jul 22, 2025
…tory::dontExpandRelationshipsByDefault()` (laravel#56154)

* default expanding relationships static property

* flush

* flush in test

* static method

* formatting

---------

Co-authored-by: Taylor Otwell <taylor@laravel.com>
itinerare pushed a commit to itinerare/Mundialis that referenced this pull request Jul 23, 2025
This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [laravel/framework](https://laravel.com) ([source](https://github.com/laravel/framework)) | `12.20.0` -> `12.21.0` | [![age](https://developer.mend.io/api/mc/badges/age/packagist/laravel%2fframework/12.21.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/packagist/laravel%2fframework/12.21.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/packagist/laravel%2fframework/12.20.0/12.21.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/packagist/laravel%2fframework/12.20.0/12.21.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) |

---

### Release Notes

<details>
<summary>laravel/framework (laravel/framework)</summary>

### [`v12.21.0`](https://github.com/laravel/framework/blob/HEAD/CHANGELOG.md#v12210---2025-07-22)

[Compare Source](laravel/framework@v12.20.0...v12.21.0)

- fix(vite): [#&#8203;55793](laravel/framework#55793) add explicit as-script to link tag for script modul… by [@&#8203;midsonlajeanty](https://github.com/midsonlajeanty) in laravel/framework#55794
- \[12.x] Allow globally disabling Factory parent relationships via `Factory::dontExpandRelationshipsByDefault()` by [@&#8203;cosmastech](https://github.com/cosmastech) in laravel/framework#56154
- \[12.x] Adds checking if a value is between two columns by [@&#8203;DarkGhostHunter](https://github.com/DarkGhostHunter) in laravel/framework#56119
- \[12.x] Ensure database connection is always restored by [@&#8203;xurshudyan](https://github.com/xurshudyan) in laravel/framework#56258
- \[12.x] Fix handling of `Htmlable` objects in `Js::convertDataToJavaScriptExpression()` by [@&#8203;jj15asmr](https://github.com/jj15asmr) in laravel/framework#56253
- Reduce meaningless intermediate variables. by [@&#8203;LjjGit](https://github.com/LjjGit) in laravel/framework#56265
- \[12.x] Improve typehints for `AbstractCursorPaginator@through()` by [@&#8203;cosmastech](https://github.com/cosmastech) in laravel/framework#56267
- Use `Date` facade instead of `time()` for `password_confirmed_at` check by [@&#8203;dylanbr](https://github.com/dylanbr) in laravel/framework#56270
- \[12.x] fix: Collection::transform() and Paginator::through() return types by [@&#8203;calebdw](https://github.com/calebdw) in laravel/framework#56273
- \[12.x] Merge 11.x into 12.x by [@&#8203;u01jmg3](https://github.com/u01jmg3) in laravel/framework#56289
- \[12.x] Reduce meaningless intermediate variables by [@&#8203;AhmedAlaa4611](https://github.com/AhmedAlaa4611) in laravel/framework#56288
- \[12.x] Refactor build Method to Use Null Coalescing Assignment for Default C… by [@&#8203;Ashot1995](https://github.com/Ashot1995) in laravel/framework#56283
- \[12.x] minor code formatting improvements by [@&#8203;browner12](https://github.com/browner12) in laravel/framework#56296
- \[12.x] Use more specific route binding exception message for child routes by [@&#8203;jessekoerhuis](https://github.com/jessekoerhuis) in laravel/framework#56298
- \[12.x] Fix Possible Undefined Variables by [@&#8203;calfc](https://github.com/calfc) in laravel/framework#56292
- \[12.x] Fix: Ensure scheduler `dailyAt()` method parses minutes and ignores seconds when seconds are provided by [@&#8203;amirhshokri](https://github.com/amirhshokri) in laravel/framework#56308
- \[12.x] Allows for strict boolean validation by [@&#8203;peterfox](https://github.com/peterfox) in laravel/framework#56313
- Improve `SeedCommand` console output by [@&#8203;Jehong-Ahn](https://github.com/Jehong-Ahn) in laravel/framework#56310
- \[12.x] Add unified enum support across framework docs by [@&#8203;amirhshokri](https://github.com/amirhshokri) in laravel/framework#56271
- \[12.x] Allows for strict numeric validation by [@&#8203;peterfox](https://github.com/peterfox) in laravel/framework#56328
- \[12.x] Update PHPDoc annotations in `Validation` by [@&#8203;mrvipchien](https://github.com/mrvipchien) in laravel/framework#56321
- \[12.x] Add operator class support for PostgreSQL GiST spatial indexes by [@&#8203;joteejotee](https://github.com/joteejotee) in laravel/framework#56324
- Fix multipart array value parsing in HTTP client ([#&#8203;55732](laravel/framework#55732)) by [@&#8203;joteejotee](https://github.com/joteejotee) in laravel/framework#56302
- Fixes bug with ShouldBeUniqueUntilProcessing locks getting stuck due to Middleware by [@&#8203;TWithers](https://github.com/TWithers) in laravel/framework#56318
- \[12.x] add prompts based expectations to PendingCommand by [@&#8203;BinaryKitten](https://github.com/BinaryKitten) in laravel/framework#56260
- \[12.x] Add Singleton and Scoped attributes to Container by [@&#8203;riasvdv](https://github.com/riasvdv) in laravel/framework#56334
- Fix unsetting model castable attribute when cast to object ([#&#8203;56335](laravel/framework#56335)) by [@&#8203;guram-vashakidze](https://github.com/guram-vashakidze) in laravel/framework#56343
- \[12.x]  Fix/memory improvement by [@&#8203;CharrafiMed](https://github.com/CharrafiMed) in laravel/framework#56345
- \[12.x] Add hasMailer method to the mailable class by [@&#8203;kevinb1989](https://github.com/kevinb1989) in laravel/framework#56340
- \[12.x] Consistent use of `mb_split()` to split strings into words by [@&#8203;shaedrich](https://github.com/shaedrich) in laravel/framework#56338
- \[12.x] Add toStringable to Uri by [@&#8203;Kyrch](https://github.com/Kyrch) in laravel/framework#56359
- \[12.x] Fix PHPStan Integrations by [@&#8203;crynobone](https://github.com/crynobone) in laravel/framework#56369
- Add 'isEmpty' and 'isNotEmpty' to Fluent by [@&#8203;cworreschk](https://github.com/cworreschk) in laravel/framework#56370
- \[12.x] Add mergeMetadata method to the Mailable class by [@&#8203;kevinb1989](https://github.com/kevinb1989) in laravel/framework#56376
- Add 'dontReportUsing' to filter exceptions to be reported by [@&#8203;pelmered](https://github.com/pelmered) in laravel/framework#56361

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0MS40Mi4zIiwidXBkYXRlZEluVmVyIjoiNDEuNDMuMCIsInRhcmdldEJyYW5jaCI6ImRldmVsb3AiLCJsYWJlbHMiOlsiZGVwZW5kZW5jaWVzIl19-->

Reviewed-on: https://code.itinerare.net/itinerare/Mundialis/pulls/282
Co-authored-by: Amadeus[bot] <renovate@itinerare.net>
Co-committed-by: Amadeus[bot] <renovate@itinerare.net>
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.

4 participants