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

[5.3] app:name - add file exists check for ModelFactory.php during namespace replacement #16592

Merged
merged 3 commits into from
Dec 1, 2016

Conversation

ralphschindler
Copy link
Contributor

Fixes issue #16575

  • add file exists check to ModelFactory.php file when performing app:name command

- add file exists check to ModelFactory.php file when performing app:name command
@ralphschindler ralphschindler changed the title Hotfix/app rename [5.3] Hotfix/app rename Nov 29, 2016
@ralphschindler ralphschindler changed the title [5.3] Hotfix/app rename [5.3] app:name - add file exists check for ModelFactory.php during namespace replacement Nov 29, 2016
@GrahamCampbell
Copy link
Member

The app:name is only ever meant to be run on a new application tbh.

@ralphschindler
Copy link
Contributor Author

Agreed, but in the case someone setting up a new app happens to remove the ModelFactory.php then run app:name, it will leave the new application in a broken state because app:name did not have a chance to dump the autoload, and there is no indication to the developer that is the thing that fixes a broken app.

I think there are some legitimate use cases where someone might have trashed database/ directory or even perhaps switched to phinx, for example. If they started building migrations before touching app/ code (where you expect the namespace to be changed) then they notice they want to change the namespace and run the command.

All that said, we're really talking about AppNameCommand doing even the smallest bit of due diligence in determining if the file its trying to update is there. I think this one liner has loads of benefits for nearly zero drawbacks.

$modelFactoryFile = $this->laravel->databasePath().'/factories/ModelFactory.php';

if ($this->files->exists($modelFactoryFile)) {
$this->replaceIn($modelFactoryFile, $this->currentRoot, $this->argument('name'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be simpler to do that check directly in replaceIn function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is a good idea, ... updating PR.

- moved file existence check into the replaceIn() functionality of the command
@browner12
Copy link
Contributor

i'm curious, are there reasons that people change their App name, other than for purely cosmetic reasons? when this feature first appeared I used it on a new app, but quickly found it to be a little bit of a PITA while providing no real benefit.

if it is purely cosmetic, I would almost argue that we get rid of it.

@taylorotwell taylorotwell merged commit 11a2428 into laravel:5.3 Dec 1, 2016
@taylorotwell
Copy link
Member

I agree renaming App is generally not something I do or even recommend, but while the command is here might as well make it work I guess.

I would be open to removing it or even extracting the whole functionality into a package that is maintained separately as it is a little bit of a time sink. If anyone wants to take that on ping me!

@ralphschindler
Copy link
Contributor Author

Thanks for merging @taylorotwell.

If in the future there needs to be a discussion on keeping/removing it, I can add to the argument there - we (at work) find it beneficial for a host of reasons.

@browner12
Copy link
Contributor

hey @ralphschindler, would you mind listing some of the reasons here for documentation?

I'm also personally curious to hear what they are.

Thanks!

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.

5 participants