Support for different plugin folders for locations in UpgradeShell. #138

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
@labianchin
Contributor

labianchin commented Jul 1, 2011

So, replacing the pull request #136.
Now in only one commit.
I placed the code of updating locations in a foreach loop that updates the location for each plugin folder and also for the app dir.

@AD7six

This comment has been minimized.

Show comment Hide comment
@AD7six

AD7six Jul 4, 2011

Member

can you clarify why this is wanted/needed, the upgrade shell already processes the global and app plugins dirs.

Member

AD7six commented Jul 4, 2011

can you clarify why this is wanted/needed, the upgrade shell already processes the global and app plugins dirs.

@labianchin

This comment has been minimized.

Show comment Hide comment
@labianchin

labianchin Jul 4, 2011

Contributor

Yes, right.
The problem is when there is some customization for the plugins folder in the bootstrap.php.
And also, I think it is more correct to get paths from the App::path instead of hard coding (directly writing) the path.

Contributor

labianchin commented Jul 4, 2011

Yes, right.
The problem is when there is some customization for the plugins folder in the bootstrap.php.
And also, I think it is more correct to get paths from the App::path instead of hard coding (directly writing) the path.

@AD7six

This comment has been minimized.

Show comment Hide comment
@AD7six

AD7six Jul 4, 2011

Member

App::plugins() doesn't (or rather, shouldn't) return /plugins/ or /app/plugins - so if used, would not process the 2 most important cases. It's hard coded because "plugins" is a legacy location.

what kind of customizations are you actually referring to?

Member

AD7six commented Jul 4, 2011

App::plugins() doesn't (or rather, shouldn't) return /plugins/ or /app/plugins - so if used, would not process the 2 most important cases. It's hard coded because "plugins" is a legacy location.

what kind of customizations are you actually referring to?

@labianchin

This comment has been minimized.

Show comment Hide comment
@labianchin

labianchin Jul 4, 2011

Contributor

App::path('plugins') would return an array of folders that contains plugins.
Also, by default there are 2 plugins' folders: one in the root and one in the app folder.

The customizations I refer is that made by App::build().
For example:
App::build(array(
'plugins' => array('/full/path/to/plugins/', '/next/full/path/to/plugins/')
));

Contributor

labianchin commented Jul 4, 2011

App::path('plugins') would return an array of folders that contains plugins.
Also, by default there are 2 plugins' folders: one in the root and one in the app folder.

The customizations I refer is that made by App::build().
For example:
App::build(array(
'plugins' => array('/full/path/to/plugins/', '/next/full/path/to/plugins/')
));

@AD7six

This comment has been minimized.

Show comment Hide comment
@AD7six

AD7six Jul 4, 2011

Member

you're not answering the question, I'm not asking you to quote the book. what are your specific changes.

There are checks like this looking for the name "plugins" are you just leaving the door open for your plugin folder's to not be renamed correctly: https://github.com/cakephp/cakephp/blob/2.0/lib/Cake/Console/Command/UpgradeShell.php#L501

If your plugins are left with an underscored foldername, they will be either broken or only usable with incorrect code.

the fundamental question is will the code still need changing after your changes are applied.

Member

AD7six commented Jul 4, 2011

you're not answering the question, I'm not asking you to quote the book. what are your specific changes.

There are checks like this looking for the name "plugins" are you just leaving the door open for your plugin folder's to not be renamed correctly: https://github.com/cakephp/cakephp/blob/2.0/lib/Cake/Console/Command/UpgradeShell.php#L501

If your plugins are left with an underscored foldername, they will be either broken or only usable with incorrect code.

the fundamental question is will the code still need changing after your changes are applied.

@labianchin

This comment has been minimized.

Show comment Hide comment
@labianchin

labianchin Jul 4, 2011

Contributor

Humm, sorry by not aswering the question, I'm trying to help on the development of Cake.

The changes was to allow the locations update to move files of other plugins folder.

Yes, right, I didn't notice this on Line 501. But I've tested in my application, and I thought this was alright. Please hold on this pull request a little bit more, so I can test it again.

Maybe (by this line 501), more changes woulde be needed.

Contributor

labianchin commented Jul 4, 2011

Humm, sorry by not aswering the question, I'm trying to help on the development of Cake.

The changes was to allow the locations update to move files of other plugins folder.

Yes, right, I didn't notice this on Line 501. But I've tested in my application, and I thought this was alright. Please hold on this pull request a little bit more, so I can test it again.

Maybe (by this line 501), more changes woulde be needed.

@labianchin

This comment has been minimized.

Show comment Hide comment
@labianchin

labianchin Jul 5, 2011

Contributor

Hi, I've tested it again. It is everything ok.

This reference for 'plugins' you mentined (https://github.com/cakephp/cakephp/blob/2.0/lib/Cake/Console/Command/UpgradeShell.php#L501) is useless. I think it was some legacy code. I removed in commit 68ff295.

Contributor

labianchin commented Jul 5, 2011

Hi, I've tested it again. It is everything ok.

This reference for 'plugins' you mentined (https://github.com/cakephp/cakephp/blob/2.0/lib/Cake/Console/Command/UpgradeShell.php#L501) is useless. I think it was some legacy code. I removed in commit 68ff295.

@AD7six

This comment has been minimized.

Show comment Hide comment
@AD7six

AD7six Jul 5, 2011

Member

I'm still not particuarly fond of the idea of running the upgrade shell on a folder, and finding that it has modified things outside of the folder it has been run on.

I still think I prefer:

$ cd /some/app
$ cake upgrade all
....
$ cd /plugins/r/here
$ cake upgrade all
....

will see what the rest of the team think though, thanks for your efforts

Member

AD7six commented Jul 5, 2011

I'm still not particuarly fond of the idea of running the upgrade shell on a folder, and finding that it has modified things outside of the folder it has been run on.

I still think I prefer:

$ cd /some/app
$ cake upgrade all
....
$ cd /plugins/r/here
$ cake upgrade all
....

will see what the rest of the team think though, thanks for your efforts

@markstory

This comment has been minimized.

Show comment Hide comment
@markstory

markstory Nov 10, 2012

Member

Closing as I agree with @AD7six and that commands that crawl all over the filesystem when given a certain path can create unexpected results.

Member

markstory commented Nov 10, 2012

Closing as I agree with @AD7six and that commands that crawl all over the filesystem when given a certain path can create unexpected results.

@markstory markstory closed this Nov 10, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment