Skip to content

[AssetMapper] ignore missing directory in isVendor() #58859

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

Conversation

alexislefebvre
Copy link
Contributor

@alexislefebvre alexislefebvre commented Nov 13, 2024

Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #58858
License MIT

I don't know how to add tests yet, because this method is private.

@carsonbot carsonbot added this to the 7.2 milestone Nov 13, 2024
@alexislefebvre alexislefebvre force-pushed the 6.4-ignore-missing-directory-in-isVendor branch from 40ac31b to d38fc98 Compare November 13, 2024 17:38
@alexislefebvre alexislefebvre changed the base branch from 7.2 to 6.4 November 13, 2024 17:39
@alexislefebvre alexislefebvre changed the title fix: ignore missing directory in isVendor() fix: ignore missing directory in isVendor() Nov 13, 2024
@alexislefebvre alexislefebvre changed the title fix: ignore missing directory in isVendor() [AssetMapper] ignore missing directory in isVendor() Nov 13, 2024
@alexislefebvre alexislefebvre force-pushed the 6.4-ignore-missing-directory-in-isVendor branch 2 times, most recently from 74495f5 to 88ab9b0 Compare November 14, 2024 16:26
@OskarStark OskarStark modified the milestones: 7.2, 6.4 Nov 14, 2024
@smnandre
Copy link
Member

Arf sorry.... i did not see it :(

#58880

@carsonbot carsonbot changed the title [AssetMapper] ignore missing directory in isVendor() ignore missing directory in isVendor() Nov 14, 2024
@alexislefebvre
Copy link
Contributor Author

alexislefebvre commented Nov 15, 2024

Arf sorry.... i did not see it :(

#58880

Your PR relies on a simple mock, while my PR changes $this->createFactory(…);. Your test looks cleaner, should we use it in this current PR instead of changing createFactory()?

@carsonbot carsonbot changed the title ignore missing directory in isVendor() [AssetMapper] ignore missing directory in isVendor() Nov 15, 2024
@smnandre
Copy link
Member

Arf sorry.... i did not see it :(
#58880

Your PR relies on a simple mock, while my PR changes $this->createFactory(…);. Your test looks cleaner, should we use it in this current PR instead of changing createFactory()?

No idea 🤷

But if you want to take anything from my closed PR you can of course :)

@alexislefebvre
Copy link
Contributor Author

No idea 🤷

But if you want to take anything from my closed PR you can of course :)

Thanks for the feedback, let's see the opinions of the reviewers. 🙂

@symfony symfony deleted a comment from carsonbot Nov 20, 2024
@nicolas-grekas nicolas-grekas force-pushed the 6.4-ignore-missing-directory-in-isVendor branch from 88ab9b0 to 9e3984f Compare November 20, 2024 11:01
@nicolas-grekas
Copy link
Member

Thank you @alexislefebvre.

@nicolas-grekas nicolas-grekas merged commit 10be4d6 into symfony:6.4 Nov 20, 2024
7 of 9 checks passed
@nicolas-grekas
Copy link
Member

Less mocks FTW

@alexislefebvre alexislefebvre deleted the 6.4-ignore-missing-directory-in-isVendor branch November 20, 2024 13:00
@fabpot fabpot mentioned this pull request Nov 27, 2024
This was referenced Nov 27, 2024
nicolas-grekas added a commit that referenced this pull request Jan 7, 2025
…lexislefebvre)

This PR was merged into the 7.3 branch.

Discussion
----------

[AssetMapper] use constants in MappedAssetFactoryTest

| Q             | A
| ------------- | ---
| Branch?       | 7.3
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        | no
| License       | MIT

This avoids repeating `__DIR__` in several places.

Follow-up of:

- #58859

Commits
-------

cdb3080 feat: use constants in MappedAssetFactoryTest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[AssetMapper] MappedAssetFactory->isVendor() return true when the directory is missing
7 participants