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.

Sign up for GitHub

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

Jump to bottom

remove default template loading on exception #3704

Merged
merged 1 commit into from
May 17, 2016
Merged

remove default template loading on exception #3704

merged 1 commit into from
May 17, 2016

Conversation

greg0ire
Copy link
Contributor

@greg0ire greg0ire commented Apr 4, 2016

Why would anyone want to rely on that? Maybe there is a valid reason,
but it does not appear in the comments or in the commit history. The
best way to find out is to remove the "feature". If anyone reverts this
commit, please make sure to add a comment to explain this.

@greg0ire greg0ire force-pushed the remove_default_template_on_exception branch 3 times, most recently from 19537e2 to fe9d29b Compare April 5, 2016 09:15
@greg0ire
Copy link
Contributor Author

greg0ire commented Apr 5, 2016

I have run my project's test suite with this piece of code, and it passed. I'll keep it in my project for a while, but I think this really can be removed.

@greg0ire greg0ire mentioned this pull request Apr 5, 2016
Merged
@greg0ire greg0ire added the major label Apr 13, 2016
@greg0ire
Copy link
Contributor Author

This is that kind of BC-break :

xkcd

@pulzarraider
Copy link
Contributor

pulzarraider commented Apr 25, 2016 •
edited
Loading

Refs #3255.

Even this is BC break, I am 👍 for this. Debugging this kind of typo in template can be very paintfull.
To not create BC break, the existing code should be kept, but some exception should occur in dev mode.

@greg0ire
Copy link
Contributor Author

Debugging this kind of typo in template can be very paintfull.
To not create BC break, the existing code should be kept, but some exception should occurs in dev mode.

Yes, but I don't really want to keep BC.

@soullivaneuh soullivaneuh added this to the 4.0 milestone May 8, 2016
@soullivaneuh
Copy link
Member

Please fill your PR of missing parts indicated by the "Needs" labels.

Feel free to ask if you want some precision of what is needed.

@greg0ire greg0ire force-pushed the remove_default_template_on_exception branch from fe9d29b to b0ed832 Compare May 11, 2016 20:42
@greg0ire
Copy link
Contributor Author

@soullivaneuh done

@greg0ire greg0ire force-pushed the remove_default_template_on_exception branch from b0ed832 to c63ecc0 Compare May 13, 2016 21:12
@greg0ire
Copy link
Contributor Author

@sonata-project/contributors final review ?

@soullivaneuh
Copy link
Member

Personally, I would like a BC way with a trigger_error.

If we can follow the Symfony method (new major is the last minor without deprecated stuff), it would be wonderful. 👍

In any case, an upgrade note is needed.

@greg0ire
Copy link
Contributor Author

Ok, I'll do that.

@greg0ire
Copy link
Contributor Author

Temporarily closing so that this does not get merge by error.

@greg0ire greg0ire closed this May 14, 2016
@greg0ire greg0ire reopened this May 16, 2016
@greg0ire greg0ire force-pushed the remove_default_template_on_exception branch 4 times, most recently from d5d2783 to 5e20857 Compare May 16, 2016 18:35
greg0ire
greg0ire reviewed May 16, 2016
View reviewed changes
Tests/Twig/Extension/SonataAdminExtensionTest.php

/**
* @expectedException Twig_Error_Loader
* @expectedExceptionMessage Unable to find template "base_list_nonexistent_field.html.twig"
* @group legacy
* @expectedExceptionMessage Unable to find template "list_nonexistent_template.html.twig"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure whether this test should be kept… it depends on Twig.

@greg0ire
Remove default template loading on exception
b045372 
Why would anyone want to rely on that? Maybe there is a valid reason,
but it does not appear in the comments or in the commit history. The
best way to find out is to remove this "feature". If anyone reverts this
commit, please make sure to add a comment to explain this.
@greg0ire greg0ire force-pushed the remove_default_template_on_exception branch from 5e20857 to b045372 Compare May 16, 2016 18:37
@greg0ire
Copy link
Contributor Author

ping @sonata-project/contributors

@soullivaneuh soullivaneuh merged commit 001d575 into sonata-project:master May 17, 2016
@greg0ire greg0ire deleted the remove_default_template_on_exception branch May 17, 2016 09:08
@greg0ire greg0ire mentioned this pull request Jul 25, 2016
Closed
@greg0ire greg0ire mentioned this pull request Aug 1, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
major
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
4 participants
@greg0ire @pulzarraider @soullivaneuh @SonataCI