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

Implement Composer #50

Closed
jarednova opened this issue Aug 26, 2013 · 36 comments
Closed

Implement Composer #50

jarednova opened this issue Aug 26, 2013 · 36 comments
Assignees

Comments

@jarednova
Copy link
Member

No description provided.

@bryanaka
Copy link

What exactly do you want to use Composer for? Like to publish to composer as a package? To state that it depends on twig and dev-dep on phpunit? or all the above, etc.

@jarednova
Copy link
Member Author

good question; a few people asked about adding Composer which is what I was reacting too. My process doesn't involved Composer so I wanted to familiarize myself with it and add the necessary pieces. Do you already know it well?

@bryanaka
Copy link

Yeah, I've used a Composer when I was working more heavily in PHP. In a nutshell, it is sorta similar to Rubygems, in that Composer manages dependancies of your project (Gemfile) and generates an autoload for you (Bundler manages load paths in ruby).

Honestly, I don't see the benefit of adding it as a package to composer. The cms handles updates, installation, and loading for you.

Maybe for development, just adding a composer.json file to manage the dependency on PHPUnit and Twig? I would hesitate on twig though, since it is a core dependency, and not all people using your plugin are composer-savvy. PHPUnit on the other hand, is a developer tool and should probably be managed.

@bryanaka
Copy link

NPM (node package manager) is similar as well, except NPM loads modules with CommonJS's require function. Composer just auto-loads with the PHP SPL

@Anahkiasen
Copy link

Composer works with Wordpress (see WPackagist ) but the main file (timber.php) isn't currently autoloaded by Composer, it needs to be registered in the composer.json.

@jarednova
Copy link
Member Author

@Anahkiasen can you send attach a sample of what that composer.json should look like (or better yet fork and send a pull request)

@Anahkiasen
Copy link

Of course yes, thanks for the wonderful package by the way :p

@bryanaka
Copy link

@Anahkiasen do you feel WPackagist really helps your workflow? I think Wordpress lacks a inter-plugin dependency system, and this would sorta solve it. How does this compare to using something like WP-CLI?

I ask because I am brand new to Wordpress, so I probably haven't run into a lot of the problems veterans have.

@mgmartel
Copy link

My 2c: in WP development, I use composer mainly for generating an autoloader and sometimes for non-wp dependencies (like loggers, or in this case, Twig). But if you ask me, once you go PSR-0, you never want to go back..

Simple example of what this would entail for Timber:

// composer.json
{
    "repositories": [ // Define repo for PHP Router  ],
    "require": {
        "fapbot/twig": "1.14.0",
        "dannyvankooten/PHP-Router" :  'dev-master',
        "asm89/twig-cache-extension" : '*'
    },
    "autoload": {
        "psr-0": {
            "Timber\\": "src/"
        }
    }
}

The file structure would then become something like:

timber/
    src/
        Timber/
            Timber.php // main plugin file
            TimberLoader.php
            ...
    vendor/
        composer/ // Composer autoloading class
        fabot/
            twig/
        dannyvankooten/
            PHP-Router/
        autoload.php
    timber.php // main plugin file
    composer.json
    ...

One gotcha is that the check for PHP version should happen in the main plugin file, before the autoloader is included, as namespaces only come in at PHP >= 5.3.0.

Another is that this entails that all classes will be namespaced.. so anybody currently using (or extending) TimberPost, for example, will be in trouble and has to change to \Timber\TimberPost. One solution for backwards compatibility would be to include all those classes the ol'-fashioned way - but that kind of defeats the purpose. Another would be to create class aliases for all those classes, so at least in a couple of next versions, the root-namespace classes can still be used.

@mgmartel mgmartel mentioned this issue Oct 18, 2013
@Anahkiasen
Copy link

They don't have to be namespaced, Composer supports all types of autoloading besides PSR0. It's ideal in most case but the Wordpress community isn't used to namespaces (and a lot of other things PHP now uses) so I'd say it'd be best to use a classmap autoload directive.
It would be infinitely better for dependencies though like Twig yes, as it would be a lot easier to keep them up to date and autoload them.

@mgmartel
Copy link

You're right about alternative autoloading @Anahkiasen. And yes, in WP namespacing is still quite uncommon, also because WP still supports PHP5 <= 5.3. But since Timber already required a PHP version that supports namespaces and uses modules that are namespaced, why not use namespaces and PSR-0 for everything Timber?

Maybe a solution is to add (yet) another autoloader that creates aliases on-the-fly for classes that start with 'Timber'. So class_exists('TimberLoader') automatically calls class_alias('\Timber\TimberLoader','TimberLoader');, etc. (Will this work? Never tried it..)

Or are there other reasons for sticking to non-namespaced classes?

@Anahkiasen
Copy link

Well if you add an autoloader like that you're basically having unamespaced classes but with worse performances. It's not that much of a problem as classes are already "name"spaced (ie. TimberLoader instead of Loader etc). Timber uses dependencies with namespaces but the user doesn't have to deal with them, while they do have to deal with Timber classes. Considering on most WP tutorials half of the people don't know what a Closure or namespace is, I wouldn't risk it. Some plugins even break when you use namespaces, that's not really a good sign.

@mgmartel
Copy link

Yep, performance of "name"spaced classes will suffer with class aliases in an autoloader - but this would just be a temporary measure to ensure backward compat with themes that use the current classes. The current "name"spaces, however, do solve most of the problems that namespaces should solve as well. So keeping them is a valid option.

Developer ignorance is a good argument for sticking to "name"spaces. However, it is a double edged sword: a couple of years ago, most plugins were completely functional, and using OOP could have been considered harmful, as most WP tutorials out there preached the use of functions. Some plugins unfortunately still do this (I'm looking at you, Akismet).
(I remember a long Google session way back, trying to find out how to add class methods to actions and filters!)

It's a tough choice, and a matter of taste. But considering that Timber is there to advance WP development and bring it up to standards that are considered default on most modern PHP projects, I'd still lean towards namespacing all of it.

(Ps. @Anahkiasen, off topic: I didn't know of any plugins breaking when using namespaces.. any idea why that is?)

@Anahkiasen
Copy link

Well I'm talking about plugins breaking when the user's classes are using namespaces, in the case it was the JSON API plugin if I recall.

You do make a good point about Timber being kind of a "top of the lot"/outsider in terms of the PHP knowledge it requires and teaches its user (which is why I used it in the first place). Hadn't thought about that. I think that kind of changes it yeah, when you see it in that light maybe namespaces would be a good thing.

@bryanaka
Copy link

Most hosting providers seem to give PHP 5.3+ support, including the popular ones shown on wordpress.org. Namespaces breaking stuff is an edge case that can/should be tested for (I am learning PHP Unit right now so I can contribute a bit on this front). IMHO it's better than what I consider the dumb approach of "prefix all the things" function approach... But thats another rant 😉 - http://i.imgur.com/ua7Vgg3.jpg

I agree that Timber is sort of an outlier when it comes to WP development. I stumbled upon it when searching for twig templating for Wordpress. Chances are, unless a dev has experience building non-trivial, non-wp applications, they probably don't really come at things from the MVC/logic-view separation mindset and do what most tutorials show. And the devs that do happen upon Timber who don't know concepts like PSR-0 or namespaces, will come out a better dev learning about it. I'm all for pushing the PHP community forward where possible.

On a practical note though, from the public API, these things will matter very little. The Timber Object is really just using a composition pattern, so changes to anything else would be encapsulated in that object for the most part.

@jarednova
Copy link
Member Author

Hey all, I'll confess I'm a little new to this world. I think I'm one of those WP heads who wants to do better than wp_every_single_fucking_thing so this is my first introduction to some concepts like PSR-0. The key part I find really important is the public API that @bryanaka mentioned. To get WP users to adopt more sophisticated concepts, I think that it's key to introduce them over time instead of in a torrent.

In terms of the organization tough; that's a piece that I'm really interested in learning more about the various options that will keep this playing nice with things like Composer and other dependency managers, etc.

@sprankle
Copy link

Ideally composer would be installed through packagist while the timber plugin would be installed through the wpackagist [http://wpackagist.org] and the two communicate. That way if WordPress is working beside a separate instance of twig (Silex, Symfony, etc) then you can use a single library and just add in the plugin.

@jarednova
Copy link
Member Author

@mtsprankle I think I follow -- did you mean that Twig (not composer) is installed through packagist?

@Anahkiasen
Copy link

Twig would be added as a dependency of Timber yes, and kept up to date like that.

@jarednova
Copy link
Member Author

Doing some clean-up. For composer users, does the current setup satisfy? I posted to packagist a few months back and it's being (sporadically) downloaded. Anything else here to worry about?

@bryanaka
Copy link

I think Twig and Twig Cache should probably be included as dependancies of Timber. I don't think it is from looking at the composer file.

@Rarst
Copy link
Contributor

Rarst commented Jan 13, 2014

For composer users, does the current setup satisfy?

I pull requested some tweaks to composer.json.

For "serious" conversion I would say Twig and any other dependencies should be converted to such in Composer terms. However that makes Composer primary install method rather than checking out repository, so depends on your goals and target users.

@Anahkiasen
Copy link

They can be added as dependencies via Composer and then track vendor. It's not best practices but it's Wordpress so, it'll still be batter and more maintainable than the currrent setup.

@jarednova jarednova self-assigned this May 12, 2014
@jarednova
Copy link
Member Author

So I'm taking this on this week to close this up. The big thing is to separate the code here on GitHub as the composer-fied development version and the WordPress.org version as the fully-packed one-click development version. Here are the steps I'm going through:

  • Declare all dependencies in composer.json so that they load into /vendor
  • Remove any external libraries that are a part of repo
  • Gitignore the vendor directory.
  • Make the new requirements very clear in readme (should be no more complicated than running composer install, but still an important step)
  • Deploy to WordPress.org with the vendor directory intact

There might be more composer-related changes to make down the road, especially related to autoloading, PSR-0, etc. I think those are best handled as separate forthcoming tickets.

@jarednova
Copy link
Member Author

I have this pretty much ready. The only exception is the CacheExtension from @asm89. It's loading in via composer, however I didn't fully understand how your autoloader functions together with other files. @mgmartel, can you take a minute to refactor so it pulls in the composer version?

https://github.com/jarednova/timber/tree/50_composer

... I think this is following the correct pattern for composer loading. But if you have an alternate recommendation I'm all ears. Once this is complete, I hope to move-on to more advanced stuff with PSR-0, etc. (which I can see you already have going in parts of the cache).

@asm89
Copy link

asm89 commented May 12, 2014

@jarednova Can you open a PR with your changes? Makes it easier to comment/add suggestions.

Didn't know that this plugin uses my cache extensions btw, cool! :)

@jarednova
Copy link
Member Author

Hey @asm89 welcome to the party! I don't think there are any actual changes, more of an implementation we're trying to get right. I'll defer to @mgmartel if there is something to PR back to you though -- he worked on the caching elements of the code. Thanks for lending your code! I'm really hoping to popularize Twig with the WordPress world.

@asm89
Copy link

asm89 commented May 13, 2014

@jarednova I meant with your changes to get composer working with this repository (since you're asking feedback on https://github.com/jarednova/timber/tree/50_composer). A PR would make it easier to comment and provide feedback.

It seems you're doing a decent job popularizing Twig in the WordPress world. :)

@jarednova
Copy link
Member Author

@asm89 -- ahh sorry, that makes more sense. The PR is now ready as #227

@jarednova
Copy link
Member Author

This is now all pulled into master. I'm going to wait a few days to sort-out any feedback and issues and then release to WordPress.org

@Rarst
Copy link
Contributor

Rarst commented May 20, 2014

  1. Add to Packagist, simplifies installation and using as dependency.
  2. If package is required as dependency it won't have "local" autoload, so include_once(__DIR__ . '/vendor/autoload.php') won't work. It needs to be conditional on if autoload exists there (otherwise just leave autoload handling to the stack).
  3. Twig version required (1.15.1) is too specific, loosen it up to something like ~1.15 or it won't play nicely with other packages using Twig.

@jarednova
Copy link
Member Author

Hey @Rarst thanks for the feedback....

  1. Up on packagist! However, I notice the "source" points to the most recently updated branch (right now, a branch where I'm trying to fix a bug with the image library) instead of master. Is this how it should be?
  2. This one I might need a little more help on. Are you saying I need to do a file_exists-style test first? something like...
$autoload = __DIR__ . '/vendor/autoload.php';
if (file_exists($autoload)){
     include_once($autoload);
} else {
     trigger_error('you need to run composer install first');
}

Three. done!

@jarednova jarednova reopened this May 20, 2014
@Rarst
Copy link
Contributor

Rarst commented May 20, 2014

I notice the "source" points to the most recently updated branch

Not sure what you mean. If header on top (before issues and wiki) that's pretty informational, it's just for human reference. It points to master for me right now.

On 2 — yes, sans "else" part. If you create Timber as standalone project autoload would be in /wp-content/timber/vendor and it needs to be loaded. However if Timber is included as dependency in whole-site stack then vendor (and autoload in it) might be something like /vendor or /wp-content/vendor and it won't be Timber's responsibility to load that autoload (shared with all other packages).

@jarednova
Copy link
Member Author

Awesome, that's good-to-go. In terms of the packagist question, here's what's featured on there now since I pushed a patch to a branch for an issue:

screen shot 2014-05-20 at 6 15 05 pm

... you can see that source is now listed as https://github.com/jarednova/timber/tree/235_sidebar_output. While it's a mighty-fine branch, I'd want this to remain master, no?

@Rarst
Copy link
Contributor

Rarst commented May 20, 2014

I imagine you can probably specify that explicitly in support section of composer.json, but really there is hardly reason to use that link in practice for anyone. :)

@jarednova
Copy link
Member Author

cool, the rest of your items are taken care of. Calling this one closed

jarednova added a commit that referenced this issue May 21, 2014
* master:
  removed "public" keyword -- methods in question are public by default
  fixes #230
  adding test for short codes close #125
  added source to composer.json as recommend by @Rarst in #50
  test for autoload file before inclusion from composer ref #50
  changed Twig requirement to ~1.15
jarednova added a commit that referenced this issue May 21, 2014
* master:
  removed "public" keyword -- methods in question are public by default
  fixes #230
  added link to TidyRepo
  adding test for short codes close #125
  added source to composer.json as recommend by @Rarst in #50
  test for autoload file before inclusion from composer ref #50
  changed self to TimberImageLibrary instead of action hook
@mgmartel mgmartel mentioned this issue Mar 1, 2015
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

No branches or pull requests

7 participants