Skip to content
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

[Asset] Missing manifest file throws an exception #31124

Open
wants to merge 1 commit into
base: master
from

Conversation

@Simperfit
Copy link
Contributor

commented Apr 16, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #27033
License MIT
Doc PR symfony/symfony-docs#11462

This PR takes what is @Seikyo saying, updated all the tests and add the parameters, I've decided to change the name of the parameter, tell me if it's ok.

@Seikyo

This comment has been minimized.

Copy link

commented Apr 16, 2019

Thank you for your time and the PR !
I totally agree with the default state of raising an exception and only being silent when manually setting it in the configuration.

@Simperfit

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

@nicolas-grekas do you agree with @OskarStark that should be treated as a bugfix ?

@Simperfit Simperfit force-pushed the Simperfit:feature/allow-missing-json-files-in-assets branch from 674aac0 to 35105ce Apr 16, 2019

@OskarStark

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

IMO this is no bugfix, so master branch is right, but in this case 4.2 section in the changelog is not the right place

Argh forget it, this is just a typo fix the new feature is listed above, so everything is fine 👍🏻

Greetings from the hospital 🏥🤣🙈 drugs... 😅

@Simperfit Simperfit force-pushed the Simperfit:feature/allow-missing-json-files-in-assets branch from 35105ce to bc4647f Apr 17, 2019

@Simperfit

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

Status: Needs Review

@nicolas-grekas nicolas-grekas added this to the next milestone Apr 17, 2019

@Simperfit

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

PR is ready and doc PR is done !

@stof

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

No change in FrameworkExtension ? This looks suspicious to me.

@Simperfit

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

@stof You are right I missed it totally, updated.

@Simperfit Simperfit force-pushed the Simperfit:feature/allow-missing-json-files-in-assets branch 2 times, most recently from 7f7be5f to 3ded088 Apr 23, 2019

@Simperfit

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

This is ready.

@Simperfit

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

@fabpot this is ready IMHO.

@xabbuh

This comment has been minimized.

Copy link
Member

commented May 3, 2019

What about allowing a configuration like this instead?

json_manifest:
    path: '...'
    allow_missing: true
@Simperfit

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

Does that mean we need to deprecated the old key ?

@xabbuh

This comment has been minimized.

Copy link
Member

commented May 8, 2019

@Simperfit yes :)

@Simperfit Simperfit force-pushed the Simperfit:feature/allow-missing-json-files-in-assets branch 3 times, most recently from 555668d to bbe8a6d May 12, 2019

@Simperfit

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

done @xabbuh

@Simperfit Simperfit force-pushed the Simperfit:feature/allow-missing-json-files-in-assets branch from bbe8a6d to dca5b66 May 12, 2019

@Simperfit Simperfit force-pushed the Simperfit:feature/allow-missing-json-files-in-assets branch 3 times, most recently from 45ec8ee to 9a399bf May 21, 2019

@Simperfit

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

Status: Needs Review

@xabbuh

This comment has been minimized.

Copy link
Member

commented May 23, 2019

Can you rebase to make the tests pass?

@Simperfit Simperfit force-pushed the Simperfit:feature/allow-missing-json-files-in-assets branch 3 times, most recently from 11bdebd to d558d7a May 23, 2019

@Simperfit

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

@xabbuh test fixed and PR rebased

@Simperfit Simperfit force-pushed the Simperfit:feature/allow-missing-json-files-in-assets branch from d558d7a to 3925e05 May 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.