Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Improve in the UpgradeShell. #136

Merged
merged 0 commits into from Jul 1, 2011

Conversation

Projects
None yet
3 participants
Contributor

labianchin commented Jun 28, 2011

Using App::path('plugin') in loactions update, instead of only the 'plugins' folder.

Member

ceeram commented Jun 28, 2011

This change has hardcoded 'plugins', while paths you build manually could use other dirnames for instance:

App::build(array(
'plugins' => array(
APP . 'modules'. DS,
ROOT . DS . 'modules'. DS,
),
));

I think this could be usefull enhancement, but it would be even better when it takes account of alternative plugin dirnames

Contributor

labianchin commented Jun 28, 2011

Yes, that's the idea: "takes account of alternative plugin dirnames".
This is done by:
$plugins = App::path('plugins');

@ceeram ceeram and 1 other commented on an outdated diff Jun 28, 2011

lib/Cake/Console/Command/UpgradeShell.php
- $Folder = new Folder('plugins');
- list($plugins) = $Folder->read();
- foreach($plugins as $plugin) {
- chdir($cwd . DS . 'plugins' . DS . $plugin);
- $this->locations();
+ $plugins = App::path('plugins');
+ if (!$pluginsFolders)
+ $pluginsFolders = array('plugins');
+
+ foreach ($pluginsFolders as $pluginsFolder){
+ if (is_dir($pluginsFolder)) {
+
+ $Folder = new Folder($pluginsFolder);
+ list($plugins) = $Folder->read();
+ foreach($plugins as $plugin) {
+ chdir($cwd . DS . 'plugins' . DS . $plugin);
@ceeram

ceeram Jun 28, 2011

Member

to make clear what i commented, i was talking about this line

@labianchin

labianchin Jun 28, 2011

Contributor

Yeah, that's right. My bad.
Commit 788a7e4 corrects this issue.

@ceeram ceeram and 1 other commented on an outdated diff Jun 28, 2011

lib/Cake/Console/Command/UpgradeShell.php
@@ -74,16 +74,22 @@ class UpgradeShell extends Shell {
public function locations() {
$cwd = getcwd();
- if (is_dir('plugins')) {
-
- $Folder = new Folder('plugins');
- list($plugins) = $Folder->read();
- foreach($plugins as $plugin) {
- chdir($cwd . DS . 'plugins' . DS . $plugin);
- $this->locations();
+ $plugins = App::path('plugins');
+ if (!$pluginsFolders)
@ceeram

ceeram Jun 28, 2011

Member

This line would cause warning for undefined variable

@labianchin

labianchin Jun 28, 2011

Contributor

Again, my bad.
(I'm having some troubles on using git on desktop, so I'm doing directly on GitHub :))
The correction is on 7ceebfc.

@ceeram ceeram commented on an outdated diff Jun 28, 2011

lib/Cake/Console/Command/UpgradeShell.php
- $Folder = new Folder('plugins');
- list($plugins) = $Folder->read();
- foreach($plugins as $plugin) {
- chdir($cwd . DS . 'plugins' . DS . $plugin);
- $this->locations();
+ $pluginsFolders = App::path('plugins');
+ if (!$pluginsFolders)
+ $pluginsFolders = array('plugins');
+
+ foreach ($pluginsFolders as $pluginsFolder){
+ if (is_dir($pluginsFolder)) {
+
+ $Folder = new Folder($pluginsFolder);
+ list($plugins) = $Folder->read();
+ foreach($plugins as $plugin) {
+ chdir($cwd . DS . $pluginsFolder . DS . $plugin);
@ceeram

ceeram Jun 28, 2011

Member

i would think $pluginsFolder already has the full path, so you dont need to prefix it with $cwd, i have not tested your code, have you?

@ceeram ceeram commented on an outdated diff Jun 28, 2011

lib/Cake/Console/Command/UpgradeShell.php
@@ -74,16 +74,22 @@ class UpgradeShell extends Shell {
public function locations() {
$cwd = getcwd();
- if (is_dir('plugins')) {
-
- $Folder = new Folder('plugins');
- list($plugins) = $Folder->read();
- foreach($plugins as $plugin) {
- chdir($cwd . DS . 'plugins' . DS . $plugin);
- $this->locations();
+ $pluginsFolders = App::path('plugins');
+ if (!$pluginsFolders)
@ceeram

ceeram Jun 28, 2011

Member

According to CakePHP conventions if /else statements require { and } brackets

@labianchin labianchin commented on an outdated diff Jun 28, 2011

lib/Cake/Console/Command/UpgradeShell.php
- $Folder = new Folder('plugins');
- list($plugins) = $Folder->read();
- foreach($plugins as $plugin) {
- chdir($cwd . DS . 'plugins' . DS . $plugin);
- $this->locations();
+ foreach ($pluginsFolders as $pluginsFolder){
+ if (is_dir($pluginsFolder)) {
+
+ $Folder = new Folder($pluginsFolder);
+ list($plugins) = $Folder->read();
+ foreach($plugins as $plugin) {
+ chdir($pluginsFolder . $plugin);
@labianchin

labianchin Jun 28, 2011

Contributor

The DS between $pluginsFolder and $plugin is not necessary, because it is already at the end of $pluginsFolder.

@labianchin labianchin commented on an outdated diff Jun 28, 2011

lib/Cake/Console/Command/UpgradeShell.php
@@ -74,17 +74,24 @@ class UpgradeShell extends Shell {
public function locations() {
$cwd = getcwd();
- if (is_dir('plugins')) {
+ $pluginsFolders = App::path('plugins');
+ if (!$pluginsFolders){
+ $pluginsFolders = array('plugins');
@labianchin

labianchin Jun 28, 2011

Contributor

One problem is that, if App::path('plugins'); fails, using only 'plugins' is not correct. According to the line 88, the $pluginsFolders would be a full path.

Contributor

labianchin commented Jun 28, 2011

Yes, right.
Many mistakes and commits, but I'm starting to contribute. :)
Fix on commit 10be3fb.
I hope now it is better.

Member

ceeram commented Jun 28, 2011

after having some more thorough look at what this method actually is doing, your code will create endless loop as inside the foreach the method calls itself over and over again.

you should check if $cwd is in array of App::path('plugins'), and only do the foreach if its true.

Member

ADmad commented Jun 29, 2011

Please add test cases for your updates and also make sure all existing ones pass. Without that your patch won't be accepted. Also it would be better if you squash multiple commits into a single one.

Member

ceeram commented Jun 29, 2011

There are no testcases for the upgrade shell
Op 29 jun. 2011 18:54 schreef "ADmad" <
reply@reply.github.com>
het volgende:

Please add test cases for your updates and also make sure all existing
ones pass. Without that your patch won't be accepted. Also it would be
better if you squash multiple commits into a single one.

Reply to this email directly or view it on GitHub:
#136 (comment)

Member

ADmad commented Jun 29, 2011

Oops, maybe there should be :)

@labianchin labianchin merged commit b22e30c into cakephp:2.0 Jul 1, 2011

Contributor

labianchin commented Jul 1, 2011

Hi, ADmad.
Sorry, but the UpgradeShell have no test case. I agree that it is necessary, but I have no idea how to make a test case that checks if files are correctly moved.
Maybe if someone starts the test case, I can add more tests for this pull request.
Also, I have made another pull request with the corrections and also in only one commit: it's pull request #138.

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