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

[2.1.0] tenancy:install replaces routes file #187

Closed
drbyte opened this issue Oct 19, 2019 · 5 comments · Fixed by #188
Closed

[2.1.0] tenancy:install replaces routes file #187

drbyte opened this issue Oct 19, 2019 · 5 comments · Fixed by #188
Assignees
Labels
bug Something isn't working
Milestone

Comments

@drbyte
Copy link
Contributor

drbyte commented Oct 19, 2019

When running php artisan tenancy:install it will overwrite the routes/tenant.php file contents even if the file already exists.

https://github.com/stancl/tenancy/blob/cfcb2574c22a279828d08d767dde63d0c46b4ff4/src/Commands/Install.php#L56-L76

Your setup

  • Laravel version: 6.3.0
  • stancl/tenancy version: 2.1.0
@drbyte drbyte added the bug Something isn't working label Oct 19, 2019
@stancl
Copy link
Member

stancl commented Oct 19, 2019

Running the install command multiple times will cause issues and I don't want to check if every single step of installation has been done before, but checking if the routes file exists seems reasonable. Some people may create it prior to installing the package, maybe even because they're migrating from another implementation.

@drfraker
Copy link

Seems like a good default would be like laravel's artisan generators work. When you run the command it will create files if they do not exist but will never overwrite files. However you can overwrite files by using the --force flag. Then it will install over the top of anything you have installed.

For example run php artisan make:model for a model you already have. It will say model already exists

If you want I can help add this functionality.

@drfraker
Copy link

drfraker commented Oct 19, 2019

From Laravel: Illuminate\Console\GeneratorCommand.php

        // First we will check to see if the class already exists. If it does, we don't want
        // to create the class and overwrite the user's code. So, we will bail out so the
        // code is untouched. Otherwise, we will continue generating this class' files.
        if ((! $this->hasOption('force') ||
             ! $this->option('force')) &&
             $this->alreadyExists($this->getNameInput())) {
            $this->error($this->type.' already exists!');

            return false;
        }
/**
     * Determine if the class already exists.
     *
     * @param  string  $rawName
     * @return bool
     */
    protected function alreadyExists($rawName)
    {
        return $this->files->exists($this->getPath($this->qualifyClass($rawName)));
    }

@drbyte
Copy link
Contributor Author

drbyte commented Oct 19, 2019

The publishing of migrations and config already use Laravel's already-exists protections.

It's only the routes file and the substitutions in Kernel.php that are destructive/additive.
I'm not very bothered by the Kernel.php additions being doubled up, as they're easy to catch. But losing my routes file is a bit frustrating if I haven't put all my updates in VCS yet.

@stancl
Copy link
Member

stancl commented Oct 19, 2019

I agree. Also it's painless to implement, compared to scanning the Kernel for references to tenancy-specific middleware.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants