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

make:crud skeleton optimization #132

Merged
merged 4 commits into from
Mar 17, 2018
Merged

make:crud skeleton optimization #132

merged 4 commits into from
Mar 17, 2018

Conversation

sadikoff
Copy link
Contributor

Lets start from Controller::delete action. I don't think we need 2 return statements here.

Maybe it will be better to add some flash messages?

@weaverryan
Copy link
Member

Makes sense to me!

About the flash messages, I would like to add them... but it’s a bit weird because we don’t know if the user is actually printing them or not in the base template. But still, I think we should add them

@sadikoff
Copy link
Contributor Author

sadikoff commented Mar 15, 2018

I think we don't need to know if the user is printing them or not, We just add them in index and edit templates and each user will decide to use it or not.

@weaverryan
Copy link
Member

@sadikoff I'm not sure about printing the flash messages in index.html.twig and edit.html.twig... because the user will always need to remove those... as flash messages should really live in the base layout.... That's what complicates things for me.

@sadikoff
Copy link
Contributor Author

@weaverryan maybe than we can ask user to use flash or not, and inject them in base layout including something like _flash.html.twig?

@weaverryan
Copy link
Member

@sadikoff We don't have any ability to modify Twig files right now. Also, I want to avoid asking unnecessary questions.... but maybe we should ask them if they want flash messages or not.

Currently, I'm leaning towards this: we always add flash messages in the controller. Then, we can do a quick check in their base.html.twig: if we do not see the flash messages being printed there (we could do some simple string check, not perfect, but good enough), we could print out a warning that tells them they should add this to their base.html.twig.

@sadikoff
Copy link
Contributor Author

@weaverryan If you don't mind lets merge this PR. It will fix some bugs. After I will prepare PR for flash messages and we will discuss it there.

@weaverryan
Copy link
Member

Thank you Vladimir!

@weaverryan weaverryan merged commit 97841c1 into symfony:master Mar 17, 2018
weaverryan added a commit that referenced this pull request Mar 17, 2018
This PR was squashed before being merged into the 1.0-dev branch (closes #132).

Discussion
----------

make:crud skeleton optimization

Lets start from `Controller::delete` action. I don't think we need 2 return statements here.

Maybe it will be better to add some flash messages?

Commits
-------

97841c1 simplify CrudControllerWebTest (we need only check if index works)
43ce972 added tests for crud generation with entity repository
4d1e2d8 fixing #134
1f4455e skeleton template Controller optimization
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.

None yet

2 participants