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

Removing provider from your app.php is dangerous #69

Closed
ghost opened this issue Apr 28, 2016 · 13 comments

Comments

Projects
None yet
2 participants
@ghost
Copy link

commented Apr 28, 2016

Currently configuration (at least routes) is partly overriden by acacha/adminlte-laravel, and if I try to unregister Acacha\AdminLTETemplateLaravel\Providers\AdminLTETemplateServiceProvider::class provider (as suggessted #47, #63, #59), then session is recreated with each request (laravel/framework#13352).

So that Session::flash(), etc. stops working properly!

That was horrible to debug. 😄

@rargueso

This comment has been minimized.

Copy link

commented Apr 29, 2016

Same problem here. I spent 1 day trying to undo or reoverride package routes.

Invalidating this solution to avoid overwriting route made unusable for me.

¿Any ideas?

@ghost

This comment has been minimized.

Copy link
Author

commented Apr 29, 2016

@rargueso It's lucky that they are quite a few (routes).
Which one's do you need to define (overwrite)? Give an example and I will try to help you.

@rargueso

This comment has been minimized.

Copy link

commented Apr 29, 2016

My route file:

`Route::group(['middleware' => 'web'], function () {
/* Auth & register */
// Authentication Routes...
$this->get('login', 'Auth\AuthController@showLoginForm');
$this->post('login', 'Auth\AuthController@login');
$this->get('logout', 'Auth\AuthController@logout');

// Password Reset Routes...
$this->get('password/reset/{token?}', 'Auth\PasswordController@showResetForm');
$this->post('password/email', 'Auth\PasswordController@sendResetLinkEmail');
$this->post('password/reset', 'Auth\PasswordController@reset');

Route::get('/', 'HomeController@index');

});`
I want to disable /home route and /register routes. I want to override the closuse for /.

The best solution would be to turn off all package routes (and use custom), but I have not found the way to do it.

Thanks for helping!

@acacha

This comment has been minimized.

Copy link
Owner

commented May 2, 2016

Ok I see the problem (yes @tjomamokrenko is horrible to debug ;-)):

Taylor Otwell introduced a change in 23 mar:

laravel/laravel@5c30c98

So all routes added to routes.php are by default inside a web group middleware ([https://laravel.com/docs/5.2/middleware#middleware-groups])

So as Graham Campbell said on laravel/framework#13352 nesting two or more web middleware groups create a session bug: Session is recreated on every request.

I was working on some solution... Open any ideas...

@rargueso

This comment has been minimized.

Copy link

commented May 2, 2016

How about leave routes out of package? As an option i mean. If we can't override then give us option to include or not that routes. Force to have default routes without any way to revoke or override not seems a good idea.

When a better solution is over the table, we take it.

@acacha

This comment has been minimized.

Copy link
Owner

commented May 2, 2016

Finally I find why you can't override routes.php... Let me explain you

As I said on other issues routes could be overrided by changing order of ServiceProviders in config/app.php file so if you put:

App\Providers\RouteServiceProvider::class

After

Acacha\AdminLTETemplateLaravel\Providers\AdminLTETemplateServiceProvider::class

Your routes in routes.php file will override default adminlte-laravel routes.

But this now not works because I accidentally added booted in AdminLTETemplateServiceProvider

 $this->app->booted(function () {
            $this->defineRoutes();
        });

Now I will remove this and make a new version of package. Please be sure to change this lines or run composer update to upgrade the package,

@tjomamokrenko maybe you have another problem/issue added to the related here? Be sure not using group middleware 'web' in routes.php as:

Route::group(['middleware' => 'web'], function () {
    Route::auth();

    Route::get('/home', 'HomeController@index');

    Route::get('/', function () {
        return view('welcome');
    });
});

Instead use:

    Route::auth();

    Route::get('/home', 'HomeController@index');

    Route::get('/', function () {
        return view('welcome');
    });

Because 'web' middleware group is applied by default by recent Laravel versions.

@rargueso Please notify me if this changes helps you.

acacha added a commit that referenced this issue May 2, 2016

@rargueso

This comment has been minimized.

Copy link

commented May 2, 2016

Thanks a lot @acacha for your answer.

I updated to version 2.0.9 running composer update and I moved routes outside web group. All overrides are working now, but still I have 'register' routes and there is no way to remove it.

Of course I can patch that on Auth controller, but it is not a clean solution. Any idea would be apreciated.

Thanks again.

@acacha acacha added the help wanted label May 3, 2016

@acacha

This comment has been minimized.

Copy link
Owner

commented May 3, 2016

@rargueso Please give me more info or show me the code or some examples because I can't reproduce your problem I have no problems to override register route. Please check the order your services providers are loaded in config/app.php.

@rargueso

This comment has been minimized.

Copy link

commented May 3, 2016

Overrides are working, but I want to remove register routes. So instead of take default package routes I updated package to get your fix then i added to my app.php:

/* * Acacha AdminLTE template provider */ Acacha\AdminLTETemplateLaravel\Providers\AdminLTETemplateServiceProvider::class, App\Providers\RouteServiceProvider::class

routes.php (outiside web group):

` $this->get('login', 'Auth\AuthController@showLoginForm');
$this->post('login', 'Auth\AuthController@login');
$this->get('logout', 'Auth\AuthController@logout');

// Password Reset Routes...
$this->get('password/reset/{token?}', 'Auth\PasswordController@showResetForm');
$this->post('password/email', 'Auth\PasswordController@sendResetLinkEmail');
$this->post('password/reset', 'Auth\PasswordController@reset');

Route::get('/', 'HomeController@index');`

At this point, overrides working great but still got undesired routes when I run a php artisan route:list.

| | GET|HEAD | register | | App\Http\Controllers\Auth\AuthController@showRegistrationForm | web,guest |
| | POST | register | | App\Http\Controllers\Auth\AuthController@register | web,guest |

@ghost

This comment has been minimized.

Copy link
Author

commented May 3, 2016

@rargueso If you want to disable registration you may override some methods in AuthController.php:

<?php

namespace App\Http\Controllers\Auth;

use App\User;
use Illuminate\Http\Request;
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
use Validator;
use App\Http\Controllers\Controller;
use Illuminate\Foundation\Auth\ThrottlesLogins;
use Illuminate\Foundation\Auth\AuthenticatesAndRegistersUsers;

class AuthController extends Controller
{
    /*
    |--------------------------------------------------------------------------
    | Registration & Login Controller
    |--------------------------------------------------------------------------
    |
    | This controller handles the registration of new users, as well as the
    | authentication of existing users. By default, this controller uses
    | a simple trait to add these behaviors. Why don't you explore it?
    |
    */

    use AuthenticatesAndRegistersUsers {
        AuthenticatesAndRegistersUsers::showRegistrationForm as traitShowRegistrationForm;
        AuthenticatesAndRegistersUsers::register as traitShowRegister;
    }

    use ThrottlesLogins;

    /**
     * Where to redirect users after login / registration.
     *
     * @var string
     */
    protected $redirectTo = '/home';

    /**
     * Create a new authentication controller instance.
     *
     * @return void
     */
    public function __construct()
    {
        $this->middleware($this->guestMiddleware(), ['except' => 'logout']);
    }

    /**
     * Get a validator for an incoming registration request.
     *
     * @param  array $data
     * @return \Illuminate\Contracts\Validation\Validator
     */
    protected function validator(array $data)
    {
        return Validator::make($data, [
            'name' => 'required|max:255',
            'email' => 'required|email|max:255|unique:users',
            'password' => 'required|confirmed|min:6',
        ]);
    }

    /**
     * Create a new user instance after a valid registration.
     *
     * @param  array $data
     * @return User
     */
    protected function create(array $data)
    {
        return User::create([
            'name' => $data['name'],
            'email' => $data['email'],
            'password' => bcrypt($data['password']),
        ]);
    }

    /**
     * @inheritDoc
     */
    public function showRegistrationForm()
    {
        if (config('app.env') === 'production') {
            throw new NotFoundHttpException;
        }

        return self::traitShowRegistrationForm();
    }

    /**
     * @inheritDoc
     */
    public function register(Request $request)
    {
        if (config('app.env') === 'production') {
            throw new NotFoundHttpException;
        }

        return self::traitShowRegister($request);
    }
}
@ghost

This comment has been minimized.

Copy link
Author

commented May 3, 2016

@rargueso Sorry for a late answer.

@rargueso

This comment has been minimized.

Copy link

commented May 3, 2016

Thanks @tjomamokrenko

My solution to disable routes was add two methods to my Auth Controller:

public function showRegistrationForm()
{
    abort(404);
}

public function register()
{
    abort(404);
}
@acacha

This comment has been minimized.

Copy link
Owner

commented May 4, 2016

Ok thank you all for your messages. I think I can close this issue.

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.