Fixed Plugin Task to deny option of ROOT/plugins and fixed Autoloading Generation #26

DSchalla · 2015-01-10T15:18:22Z

PR related to the change: cakephp/app#206

The first commit is filtering the ROOT/plugins path from the options and exits with an error if no other path is available, I am unsure if exit; was the correct choice in a Shell Task, I would like to get some feedback regarding that.

The second commit fixed the Autoloading Generation to allow other than the default path options to be used as plugin path, the chdir adaption was required since the old version could not deal with anything but the first child of ROOT.

Any comments are appreciated.

… for menu.

DSchalla · 2015-01-10T15:28:21Z

I am unsure how to handle the test failures, could it be that the ROOT constant is not defined in the Tests which lead to the failure? Is the usage of the ROOT variable not fine or do I simply have to rename the paths in the testcases?

dakota · 2015-01-10T15:57:11Z

I don't think it's a good idea to deny baking of plugins into ROOT/plugins. I often create plugins with bake which I then publish via packagist or a local satis install.

I don't see what's wrong with baking plugins into ROOT/plugins?

DSchalla · 2015-01-10T15:59:47Z

@dakota Did you read the PR linked at top?

dakota · 2015-01-10T16:03:08Z

I did, I still don't think it's a good idea blocking baking into ROOT/plugins. Perhaps giving a warning would be the solution?

DSchalla · 2015-01-10T16:05:06Z

A warning or maybe a option flag could be used, like --composer or something like that.

ADmad · 2015-01-10T16:07:53Z

I often create plugins with bake which I then publish via packagist or a local satis install.

@dakota IMHO you should maintain separate folders for developing / publishing and using in an app.

dakota · 2015-01-10T17:18:01Z

@ADmad you're right, I'm just being lazy :)

jadb · 2015-01-10T17:47:52Z

@dakota you can still be lazy, this PR won't stop you ;)

And to suit your laziness, here's the new path ready for you to copy/paste:

cd src/Plugin

markstory · 2015-01-10T20:10:31Z

I don't think preventing people from putting code where they want is a good idea. What if there was an output-dir option that defaulted to the new path? This would let people output plugin skeletons for standalone plugins as well.

lorenzo · 2015-01-10T20:19:59Z

I like that idea @markstory

DSchalla · 2015-01-11T00:19:25Z

Then we can just remove the filter again and the app.default.php has to be adapted, since this controls how the paths are ordered when multiple exist. Or did I misunderstood something?

@lorenzo, any hints regarding the tests?

lorenzo · 2015-01-11T20:30:43Z

src/Shell/Task/PluginTask.php

@@ -235,7 +237,7 @@ protected function _modifyAutoloader($plugin, $path)
            $cwd = getcwd();

            // Windows makes running multiple commands at once hard.
-            chdir(dirname($path));
+            chdir(ROOT);


why did this change?

It looks to be logically correct to make this change; dirname($path) means dirname('plugins/)` - it's implicitly assuming the plugins dir is one level below the root.

However, I'd rather it was:

$file = $this->_rootComposerFilePath(); # <- ... chdir(dirname($file));

There's no need to use a constant.

Indeed, the changes was related to that issue. Regarding _rootComposerFilePath(), that would be an unnecessary function call since the function is also just:
"return ROOT . DS . 'composer.json';"

However, if requested I can ofc change it to use the function call. In your snippet, dirname($file) is sufficient, since we do not want to step out of the root, hu?

Please consider testing - you don't want to be modifying ROOT . DS . 'composer.json when tests run and that's why it's in a separate function (and why generally I purged constants from the code recently).

That's totally fine, I will update the code to use the function tomorrow. Thanks for your advice @AD7six

DSchalla · 2015-01-11T20:32:05Z

When using a different path than ROOT/plugins you would not switch back correct to the root folder. As example, ROOT/src/Plugin would switch back to ROOT/src, leading to a failure for the composer call.

josegonzalez · 2015-01-15T20:39:16Z

@lorenzo bump

lorenzo · 2015-01-15T20:41:19Z

tests/comparisons/Plugin/testMainUpdateComposer.json

        }
    },
    "autoload-dev": {
        "psr-4": {
-            "ComposerExample\\Test\\": "./plugins/ComposerExample/tests"
+            "ComposerExample\\Test\\": "/tmp/tests/BakedPlugins/ComposerExample/tests"


Paths should be relative as when you deploy the path will probably not be the same.

The path used above is related to the fact that the plugin path in the Test Instance is set to /tmp/tests/BakedPlugins, on a productive environment it would look like:
'"Test6\Test": "./src/Plugin/Test6/tests"

lorenzo · 2015-01-15T21:45:29Z

Thanks a lot for the effort @DSchalla, the team decided to go another way with this. Instead of disallowing baked plugins in ROOT, we decided to put them only there and have composer install plugins in vendors. That way there will be a clear separation between both, all thrid-party libraries will be in the same place and no gitignore will be required for in-app plugins.

I'm sorry you had to work on this and not get this merged, you win 100 internets from me 💪

lorenzo · 2015-01-15T21:51:38Z

Reopening as this PR was transformed to fix a couple limitations on how plugins are baked. The problem in tests still need to be solved

DSchalla · 2015-01-16T08:43:06Z

The issues with the tests should be fixed after https://github.com/DSchalla/bake/commit/48a284bb4fe6211e0d0665b03a544f78844c9e63 and https://github.com/DSchalla/bake/commit/475d5f3c71c957474dd633e39da8397edc91f6c7.

…neration Merge branch 'DSchalla/master' into master. Closes #26

markstory · 2015-01-18T01:00:10Z

Thanks 👍

DSchalla · 2015-01-18T01:00:39Z

Thanks @markstory for the merge!

DSchalla added 2 commits January 10, 2015 15:54

Filtered ROOT/plugins from Path Options and added exception ahandling…

58b5b23

… for menu.

Fixed Autoloader for Different Path Options

616ffed

rchavik mentioned this pull request Jan 11, 2015

Ignore vendor plugins cakephp/app#206

Closed

lorenzo reviewed Jan 11, 2015
View reviewed changes

DSchalla added 6 commits January 12, 2015 16:56

Using Internal Methods to find Root Path

0df3c4b

Removed ROOT/plugins filter

870efaf

Fixed Tests

d9fed66

Fixed Tests#2

1dc8ba3

Fixed Typo in Function

8668d4b

Modified FalsePath Test for Single Return + Added ZeroPath Test

dde205f

lorenzo reviewed Jan 15, 2015
View reviewed changes

lorenzo closed this Jan 15, 2015

lorenzo reopened this Jan 15, 2015

DSchalla added 2 commits January 15, 2015 22:57

Fixed Composer Test to use relative path

48a284b

Added Spaces for CS Test

475d5f3

markstory added this to the 1.0.0 milestone Jan 15, 2015

markstory self-assigned this Jan 16, 2015

markstory merged commit 475d5f3 into cakephp:master Jan 18, 2015

markstory added a commit that referenced this pull request Jan 18, 2015

Fix PluginTask to deny option of ROOT/plugins and fixed autoloader ge…

62867ed

…neration Merge branch 'DSchalla/master' into master. Closes #26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed Plugin Task to deny option of ROOT/plugins and fixed Autoloading Generation #26

Fixed Plugin Task to deny option of ROOT/plugins and fixed Autoloading Generation #26

DSchalla commented Jan 10, 2015

DSchalla commented Jan 10, 2015

dakota commented Jan 10, 2015

DSchalla commented Jan 10, 2015

dakota commented Jan 10, 2015

DSchalla commented Jan 10, 2015

ADmad commented Jan 10, 2015

dakota commented Jan 10, 2015

jadb commented Jan 10, 2015

markstory commented Jan 10, 2015

lorenzo commented Jan 10, 2015

DSchalla commented Jan 11, 2015

lorenzo Jan 11, 2015

AD7six Jan 11, 2015

DSchalla Jan 11, 2015

AD7six Jan 11, 2015

DSchalla Jan 11, 2015

DSchalla commented Jan 11, 2015

josegonzalez commented Jan 15, 2015

lorenzo Jan 15, 2015

DSchalla Jan 15, 2015

lorenzo commented Jan 15, 2015

lorenzo commented Jan 15, 2015

DSchalla commented Jan 16, 2015

markstory commented Jan 18, 2015

DSchalla commented Jan 18, 2015

Fixed Plugin Task to deny option of ROOT/plugins and fixed Autoloading Generation #26

Fixed Plugin Task to deny option of ROOT/plugins and fixed Autoloading Generation #26

Conversation

DSchalla commented Jan 10, 2015

DSchalla commented Jan 10, 2015

dakota commented Jan 10, 2015

DSchalla commented Jan 10, 2015

dakota commented Jan 10, 2015

DSchalla commented Jan 10, 2015

ADmad commented Jan 10, 2015

dakota commented Jan 10, 2015

jadb commented Jan 10, 2015

markstory commented Jan 10, 2015

lorenzo commented Jan 10, 2015

DSchalla commented Jan 11, 2015

lorenzo Jan 11, 2015

Choose a reason for hiding this comment

AD7six Jan 11, 2015

Choose a reason for hiding this comment

DSchalla Jan 11, 2015

Choose a reason for hiding this comment

AD7six Jan 11, 2015

Choose a reason for hiding this comment

DSchalla Jan 11, 2015

Choose a reason for hiding this comment

DSchalla commented Jan 11, 2015

josegonzalez commented Jan 15, 2015

lorenzo Jan 15, 2015

Choose a reason for hiding this comment

DSchalla Jan 15, 2015

Choose a reason for hiding this comment

lorenzo commented Jan 15, 2015

lorenzo commented Jan 15, 2015

DSchalla commented Jan 16, 2015

markstory commented Jan 18, 2015

DSchalla commented Jan 18, 2015