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

[UserBundle] Delete routing and templates from UserBundle #6782

Merged

Conversation

michalmarcinkowski
Copy link
Contributor

@michalmarcinkowski michalmarcinkowski commented Nov 18, 2016

Q A
Bug fix? yes
New feature? no
BC breaks? yes
Related tickets fixes #5859
License MIT

@tchapi
Copy link
Contributor

tchapi commented Nov 19, 2016

Can you provide a little detail as to what this does exactly ? If fixes #5859 as in "we remove the whole functionality so basically this is fixed" ? What happens then with the reset password stuff, do we have to code it ourselves until you decide to put it back again ?

@michalmarcinkowski
Copy link
Contributor Author

@tchapi I don't understand your frustration... 😕 Did you have a look at the changes? This PR removes not really useful templates (that are not even tested) and routing (that is not tested also) and doesn't work correctly because of redirects to non existing routes. It fixes the Controller by removing wrong and hardcoded templates and redirects and forces to configure them in route definition.

I didn't remove any functionality, so please verify your concerns before writing such comments...

@tchapi
Copy link
Contributor

tchapi commented Nov 19, 2016

The frustration is in the explanations — for every PR that gets merged in master, we need to check and cherry-pick it to see what it does exactly and how we need to adjust our codebase before updating to the current master.

Of course we could stay at a particular commit hash but then, wait ten days and everything has changed and the migration is a nightmare. so we need to stay as close as possible to master to be able to bubble up all the changes that happen in our custom code.

It would be great if you just explained each PR with a few words, especially if you tag it "BC break". Else it's just a game of chasing tails. We're really eager and excited to use Sylius but it's complicated if we have to do this guessing game for every PR that comes by.

@michalmarcinkowski
Copy link
Contributor Author

michalmarcinkowski commented Nov 19, 2016

This week will be really hard for upgrades because we will do all BC breaks before BETA release. After the 1.0.0-beta tag, BC breaks will be done only when really necessary and all will be well documented.

We're approaching the great milestone that will hopefully end the "upgrade nightmare" for all people using Sylius, just be patient, beta incoming at the end of November, it will be worth!

@pjedrzejewski pjedrzejewski merged commit 7c00753 into Sylius:master Nov 21, 2016
@pjedrzejewski
Copy link
Member

Thank you Michał! @tchapi We are really close! :)

@michalmarcinkowski michalmarcinkowski deleted the minor-fixes-in-user-bundle branch December 3, 2016 14:36
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.

Route 'sylius_user_security_login' does not exist anymore
4 participants