Ignore vendor plugins #206

Closed
wants to merge 2 commits into
from

Projects

None yet
@jadb
Member
jadb commented Jan 8, 2015

As discussed over IRC with a couple of you guys, here is my suggestion for a better organized app with both vendor and custom plugins:

Vendor plugins are installed by composer and thus, like their vendor libraries counterparts, should not be included in the application repo.

Instead, user created plugins should go in src/Plugin which is user land created code (so to speak).

The way this was set up, one would automatically be committing in his repo DebugKit for example when it is only a dev requirement. Maybe some docs will need to be re-adjusted, etc. but all will be (IMHO) for the better.

If one wants to commit plugins into their repo (not really the 'right way') they still can by cloning the plugin in the src/Plugin or by forcing a commit of an ignored path.

@jadb jadb Ignore vendor plugins
Vendor plugins are installed by composer and thus, like their `vendor`
libraries counterparts, should not be included in the application repo.
Instead, user created plugins should go in `src/Plugin` which is user
land created code (so to speak).
e343f3b
@DSchalla
DSchalla commented Jan 8, 2015

As rather new user of CakePHP I can confirm that we ran yesterday in a few issues due to the default setup and missing experience with composer. I would appreciate the change too, in order to avoid confusion for new users, which will mainly use the AppTemplate.

@dereuromark dereuromark added this to the 3.0.0 milestone Jan 8, 2015
@ADmad
Member
ADmad commented Jan 8, 2015

@jadb You also need to add the src/Plugin path to App.plugins.path config array in app.default.php.

@jadb
Member
jadb commented Jan 8, 2015

Sorry, missed that. Fixed now.

@lorenzo
Member
lorenzo commented Jan 8, 2015

This will require a change in the bake plugin so that it only generates into the new folder

@dereuromark
Member

I wonder if that is necessary. Most can just manually add such a new folder to the ROOT dir.
It might confuse more to have Plugin and plugins there at the same time, wondering which is which.
I would just go with "git ignoring" the plugins one and document somewhere how to add such a custom second folder if really required (ideally linked from bake plugin).

//EDIT: I just saw that you actually propose to put it in src (not ROOT). That sounds better to me.

@jadb
Member
jadb commented Jan 8, 2015

@dereuromark I agree it might mix people up to see 2 plugin folders in the app template.

But maybe instructions should be about creating the second plugin folder under src and not ROOT (which is even more confusing IMO, 2 plugin folders at the same level). User-land cake code usually goes into src, no?

@dereuromark
Member

Yeah I read your proposal wrong. What you got so far sounds good! ๐Ÿ‘

@jadb
Member
jadb commented Jan 8, 2015

:)

Will look into @lorenzo's request.

@DSchalla
DSchalla commented Jan 8, 2015

@lorenzo I will take care of this, I can make a PR for the change tomorrow to allow only options which are not /plugins, so like current state but /plugins filtered.

@ceeram
Member
ceeram commented Jan 8, 2015

I like separation of composer managed plugins and those managed by the app developer
๐Ÿ‘

@markstory
Member

Should there be a different name for in-repo plugins?

@jadb
Member
jadb commented Jan 8, 2015

@markstory src/Plugin ? or u mean something else?

I thought that was inline with the rest of cake's user land architecture but as usual, I know so little compared to the rest of you :)

@dereuromark
Member

@markstory

/src/Plugin (bake)
/plugins (composer)

Same should probably be done for "vendors" vs. "Vendor", but then again people should really try to composer everything, rendering the src/Vendor dir useless :)

@ceeram
Member
ceeram commented Jan 8, 2015

@markstory wouldnt app/src/Plugin and app/plugins be different enough?

@ceeram
Member
ceeram commented Jan 8, 2015

I would not do the same thing for non cakephp-plugin libraries by re-adding app/src/Vendor. People can always put whatever they want in their applications, but plugins are a special kind of "libraries" in cake. ๐Ÿ‘Ž on doing this for vendor

@dereuromark
Member

@ceeram Also, vendors aren't baked. Just saying that there maybe should be a similar separation here too, but thats really up to each developer/application itself.

@markstory
Member

No separation of vendor code. Vendor code should be updated to pull from composer. Plugins are different as they can be used to segment large applications into more manageable pieces.

@dereuromark
Member

Well, let's ship it ๐Ÿ‘

@markstory
Member

This will need some docs changes to explain why there are two plugins directories, and why/how they differ.

@ADmad
Member
ADmad commented Jan 10, 2015

@jadb is awesome with docs too ๐Ÿ˜‰

@jadb
Member
jadb commented Jan 10, 2015

@ADmad - am gonna hunt you down!

@markstory
Member

I am not comfortable having two directories with almost the same name if there is no documentation on how they differ and what goes into each directory.

@jadb
Member
jadb commented Jan 10, 2015

@markstory so documentation is what's needed or a name change?

@markstory
Member

I think docs are more important than a rename. Even if it had a different name, we'd still need to document what that empty directory was for.

@jadb jadb referenced this pull request in cakephp/docs Jan 10, 2015
Closed

Update plugin doc #2258

@rchavik
Member
rchavik commented Jan 11, 2015

I don't see why we need to have two similar but different plugin directories. I feel this is a problem that could be solved by documentation.

๐Ÿ‘Ž to both cakephp/bake#26 and cakephp/app#206

@DSchalla

Because plugins pulled by composer get currently comitted if the application is a git repo, when you separate the folders you can put one on git ignore. The current setup is just not optimal, since you have to add each plugin installed on your own to the .gitignore.

@rchavik
Member
rchavik commented Jan 11, 2015

DSchalla, I understood the reasoning the first time. I don't agree with it. I foresee myself going into a machine and confused the hell out of myself in panic when not finding my plugins/Foo, as it turns out to be installed in src/Plugin/Foo.

@jadb
Member
jadb commented Jan 11, 2015

@rchavik if you don't see the advantages, I'll re-suggest you read up (yes, again).

I am sure that someone with your know-how will quickly adapt to the change and not get confused the way you are describing it ;)

To be honest, I don't see why one would prefer having to manually add (that's how it is now) every composer cake plugin installed to .gitignore when using the plugins directory as the only plugin directory. You seem to though and that's why markstory has suggested the option to be added to bake.

IMO, people use cake to make things FASTER, not have to redo the same thing over and over again. So, if you have a suggestion on how to not repeat the same task every time one needs to add a plugin, please share.

@jadb
Member
jadb commented Jan 11, 2015

Oh and by the way, the Foo plugin you might panic over will still be in plugins.

Simply because:

a - you do not update your app skeleton after you have started your application development - so the changes in this PR will not affect a running site you have
b - you seem to have used composer to install the plugin (since you don't know where it was installed, or otherwise you'd know it's in src/Plugin) and in that case, you don't need to panic since composer will have installed it in plugins just as you expected.

@sikker
sikker commented Jan 11, 2015

I think the idea of having a specific folder for third party plugins makes
a lot of sense. I think naming those folders virtually identically is the
wrong way to do it. I think the most elegant solution would be to have
third party plugins reside within their own composer vendor folder, but I
appreciate that sometimes people don't/can't use composer for a specific
plugin. What about, say, having a subfolder for plugins/ called
plugins/vendor, user-defined plugins residing in the root of said plugins
folder as usual?

On Sun, Jan 11, 2015 at 3:36 AM, Jad BITAR notifications@github.com wrote:

@rchavik https://github.com/rchavik if you don't see the advantages,
I'll re-suggest you read up (yes, again).

I am sure that someone with your know-how will quickly adapt to the change
and not get confused the way you are describing it ;)

To be honest, I don't see why one would prefer having to manually add
(that's how it is now) every composer cake plugin installed to .gitignore
when using the plugins directory as the only plugin directory. You seem
to though and that's why markstory has suggested the option to be added to
bake.

IMO, people use cake to make things FASTER, not have to redo the same
thing over and over again. So, unless you have a suggestion on how to not
repeat the same task every time one needs to add a plugin, please share.

โ€”
Reply to this email directly or view it on GitHub
#206 (comment).

@jadb
Member
jadb commented Jan 11, 2015

@sikker thanks for chiming in.

For the naming, I can't say for other people since it doesn't mix me up myself.

But to comment on your suggestion (and so you could iterate it maybe for one that would fix everyone's concerns), the way I see it is that cake related code goes into /src so it just makes sense that user created plugins would reside somewhere under that directory (even if under another name than Plugin). And plugins managed by composer should either be under the default /vendor (just like any other library) or then in /plugins (or any other name, but outside of src).

I keep wondering why so much mix up over the names when right now, anyone installing non-composer managed PHP libraries are most likely installing them under /srv/Vendor or some other user created directory that means vendor, just not managed by composer. Food for thought everyone.

@rchavik
Member
rchavik commented Jan 11, 2015

So, unless you have a suggestion on how to not repeat the same task every time one needs to add a plugin, please share.

Use a derived app template containing your own customizations.

@jadb
Member
jadb commented Jan 11, 2015

Thanks! I rest my case.

@AD7six
Member
AD7six commented Jan 11, 2015

FWIW I'm somewhat uneasy with these changes; my unease is not because of the changes themselves, but the underlying reason they are needed. Why do we need all of these concepts:

  • 3rd party vendor
  • 3rd party plugin
  • in-app plugin

I feel the concept of a plugin is sort of blury, and almost a snowflake. It aught to be in principle:

  • 3rd party anything === vendor
  • in app plugin === a namespace (or the current concept of a plugin)

Effectively, the only reason we have a plugins folder at all is to know where the plugin source files are to include config files and source the webroot (if it's not symlinked into the application webroot). Were it possible to do this:

DebugKit\Plugin::bootstrap();

instead of:

Plugin::load('DebugKit');

then where a plugin is located becomes irrelevant (or simply auto-discovered) and the need for a plugins folder almost disappears.

@ADmad
Member
ADmad commented Jan 11, 2015

DebugKit\Plugin::bootstrap();

That sounds interesting to me.

@jadb
Member
jadb commented Jan 11, 2015

@AD7six I like your proposition (never really liked having composer managed plugins outside of vendor nor knew it was even discussable).

Quick thoughts:

  • how would one bake a new plugin? what folder you suggest putting non-composer managed plugins for users in (when they bake).
  • would that also deprecate the plugin installer? (i think so)
  • would that force every user to have a vendor name for their plugin?

And as a final thought (maybe not aligned with the rest), wouldn't that be a non-BC change (for RC I mean).

Thanks!

@AD7six
Member
AD7six commented Jan 11, 2015

@jadb I wonder how many devs familiar with composer find that CakePHP plugin dependencies aren't in vendors confusing or at least odd - I suspect "quite a few".

My current implementation idea is:

When baking ask users where to put the plugin

OR ask the user whether it's to become a distributed plugin.

  • Distributed? no type in composer.json, located in vendors/name/here (Or skip this case entirely and document how to distribute a plugin)
  • Not distributed? located in plugins/here and either:
    • type = cakephp-plugin in the composer file
    • no composer file at all and just edit the app's autoload settings

Add a trait for plugin bootstrapping class

something like this:

namespace Cake\Core;

trait PluginTrait
{
    public static function bootstrap($options = [])
    {
        $path = static::path();
        Plugin::load(
            basename($path),
            [
                'path' => $path
            ] + $options
        );
    }

    public static function path()
    {
        return dirname(__DIR__) . DS;
    }

    public static function hasConfig()
    {
        return is_dir(static::path() . 'config');
    }

    public static function hasTemplates()
    {
        return is_dir(static::path() . 'src' . DS . 'Templates');
    }

    public static function hasWebroot()
    {
        return is_dir(static::path() . 'webroot');
    }
}

Use the trait in baked plugins

ala:

namespace DebugKit;

use Cake\Core\PluginTrait;

class Plugin
{
    use PluginTrait;
}

Injected into the app's bootstrap:

DebugKit\Plugin::bootstrap();

Remove "type": "cakephp-plugin", from the plugin template

In-keeping with convention-over-configuration there's no real need to have a cakephp-plugin type with this proposal; the plugin is either a vendor or it's a bit of the app (and part of the app's source code).

No composer? no problem

Either use the plugin class as exactly done right now (with autoload => true) or the plugin has to be autoloaded explicitly:

// app bootstrap file
use Cake\Core\ClassLoader;
use DebugKit\Plugin;

$cl = new ClassLoader();
$cl->register();
$cl->addNamespace('DebugKit', 'wherever/DebugKit');
DebugKit\Plugin::bootstrap();

Bullet point questions

Off the top of my head:

  • Extra option to change the output path? same as now.
  • No (do you mean Cake\Core\ClassLoader or something else?). Yes/Sort of, it becomes unused.
  • No.

Obviously, avoiding a BC break is paramount at this point in time - I'm suggesting to wrap how things currently work rather than replace it.

@jadb
Member
jadb commented Jan 11, 2015

@AD7six ๐Ÿ‘ i personally love the suggestion!

And yes, I think MANY find it weird that composer would install stuff outside of vendor (just because in general it doesn't), but that's an option in composer for a reason I guess and I (admittedly blindly) just followed along with how you guys had initially planned it just because I usually try to understand some decisions before taking a stance against them - often times I end up learning a thing or 2.

@markstory
Member

Requiring each Plugin to have some boilerplate feels a bit silly to me. From what I remember, the only reason we even install plugins into /plugins with composer is to allow Plugin::loadAll() to work. If this method was removed, we could very easily install plugins into /vendor with composer.

While I recognize that nerfing this method now would be pretty crappy, requiring plugins to start implementing specific classes is equally backwards incompatible.

@darxmac
darxmac commented Jan 11, 2015

If it counts for anything, I'd be sad to work around the missing Plugin::loadAll()

Sent from my iPhone

On 11 Jan 2015, at 17:40, Mark Story notifications@github.com wrote:

Requiring each Plugin to have some boilerplate feels a bit silly to me. From what I remember, the only reason we even install plugins into /plugins with composer is to allow Plugin::loadAll() to work. If this method was removed, we could very easily install plugins into /vendor with composer.

While I recognize that nerfing this method now would be pretty crappy, requiring plugins to start implementing specific classes is equally backwards incompatible.

โ€”
Reply to this email directly or view it on GitHub.

@jadb
Member
jadb commented Jan 11, 2015

@markstory - from what I understand in AD7six suggestion, there would be a trait that is reusable by plugins in general (and could be customized I assumed for whatever plugin needs to), so which boilerplate code? Maybe I missed something and then my opinion on that suggestion might also change.

If you meant the plugin definition class that will use the trait, that could be part of templates the way I saw it.

I also feel it is more inline with how dependencies should work / be injected, no? I might completely be wrong since my real ache to start with was only being able to not have to gitignore every composer managed plugin (have no personal issues with the rest of the plugin implementation, worked great for me thus far).

@ADmad
Member
ADmad commented Jan 11, 2015

@AD7six Really like your suggestion.

@markstory If we don't use the trait AD7six has proposed we would have to specify the 'path' option when loading plugin which is under vendor folder, since it cannot be guessed from plugin name.

@ADmad
Member
ADmad commented Jan 11, 2015

Another option is we keep plugin-installer which can maintain a file which contains the plugin name to path mapping. Plugin class could use that file and avoid having the trait AD7six proposed nor would we require specifying 'path' explicitly when loading a plugin under vendor.

@markstory
Member

@darxmac Yeah, I didn't think people would be too excited about not having Plugin::loadAll() anymore.

@jadb Having to define the plugin stub in each plugin feels like boilerplate to me.

@ADmad Having a config file containing paths for plugins sounds good to me. It doesn't require new stub classes, won't break existing API's and don't require the creation of a new src/Plugin directory. The file could be a simple PHP array:

// in config/plugins.php
return [
  'DebugKit' => '/vendors/cakephp/DebugKit',
  'Contacts' => '/plugins/Contacts',
];

If a plugin cannot be found in the config file, the Plugin class can assume it is an in-repo plugin and use /plugins/$name. I can put together a proposal of how this could work. It will require coordinating changes in cakephp/plugin-installer, but those changes won't be hard either.

@markstory markstory self-assigned this Jan 11, 2015
@ADmad
Member
ADmad commented Jan 11, 2015

@markstory Yup a simple array is what I had in mind too.

@AD7six
Member
AD7six commented Jan 11, 2015

If a plugin cannot be found in the config file, the Plugin class can assume it is an in-repo plugin and use /plugins/$name. I can put together a proposal of how this could work.

I'm happy with any solution which simplifies things.

@markstory markstory referenced this pull request in cakephp/cakephp Jan 12, 2015
Merged

Strawman for vendor plugins. #5641

@AD7six
Member
AD7six commented Jan 14, 2015

I'm going to close this as there are open PRs for cakephp, app and the plugin loader (with the exception of bake, but arguably that doesn't need to change).

Thanks for prompting this discussion @jadb.

@AD7six AD7six closed this Jan 14, 2015
@davidyell

This broke my everything ๐Ÿ‘Ž

@AD7six
Member
AD7six commented Jan 15, 2015

@davidyell what change broke what in what way?

@davidyell

Hey @AD7six it was a combination of factors actually. The PR being the spark which ignited the flame so to speak. Firstly I wanted to do some work on my plugin, as I was preparing I noticed a discussion around a bug with cakephp/plugin-installers 0.0.2 which was causing issues. I updated my project and hit a fatal error whilst doing composer update. Thankfully @dakota was on hand to tag a 0.0.3 release for cakephp/plugin-installers. This then moved all my plugins into /vendor. Refreshing my application in my browser caused me to get a missing plugin error.

Eventually I tracked down that this error was because the newly created config/plugins.php path was not including all my declared plugins. So after editing that, I was still hitting issues. So I decided to update my CakePHP from the RC1 tag to the latest master.

After this update, I got a total fatal for unable to load app.php. So then I hunted down a change to the bootstrap.php file in cakephp/app repo. Once I'd updated my bootstrap file I still got a missing plugin for DebugKit.

So finally I had to edit my config/plugins.php file again to add the path for DebugKit. After that, my project started working again.

So it was really a combination of destructive changes to the cakephp/app repo and this PR in combination which caused me about 2 hours work this morning. Thankfully, everything is working now. With the maturity of the 3.x branch I often forget that it's beta and will change. I should check the repos more frequently to save myself this problem in future.

@patrickconroy

Let me know if you guys want me to open a bug or if this is the new normal. I have namespaced plugins, and when I composer install the generated config/plugins.php file uses the correct (or what I think is correct since it used to be correct) forward slash e.g. Plugin::load('Company/Plugin'); Plugin::load can't load them that way anymore apparently and won't load the plugin since it expects them to be backslashes e.g. Plugin::load('Company\Plugin');

Which slash is correct?

  1. If forward, then there's a bug.
  2. If back, then I'll update my bootstrap and be on my way.

Also, would you suggest .gitignore'ing config/plugins.php since it's generated?

Edit.. I misspoke, the generated config/plugins.php looks fine - it's that now I had to change the slashes in my config/bootstrap.php when actually doing the plugin loading.

@lorenzo
Member
lorenzo commented Jan 15, 2015

Forward slash is the one we support

@patrickconroy

Thanks @lorenzo I'll open a bug then.

@patrickconroy

Thanks for the fix, @ADmad. Just a tiny thing from me which might be a non-issue.. the generated config/plugins.php has non-standard PHP syntax from the rest of 3.0. It looks like this:

return array (
  'plugins' => 
  array (
    'DebugKit' => $baseDir . '/vendor/cakephp/debug_kit/'
  )
);

When it should probably look like:

return [
    'plugins' => [
        'DebugKit' => $baseDir . '/vendor/cakephp/debug_kit/'
    ]
];

Also, the question about that I had - should it be in .gitignore? Or is it a file that should only be created after composer install instead of composer update ?

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