Skip to content

Support lowercased phakefile filenames. #29

Open
wants to merge 5 commits into from

3 participants

@ilkerde
ilkerde commented Aug 28, 2013

Support lowercased runfile names to be recognized as well. Example:

phakefile
phakefile.php

Tests for proper file recognition added aside.

PHPUnits bootstrap now uses composers autoloader in order to benefit
from autoloading for test classes as well. The composer.json file
reflects PHPUnit and vfsStream dependencies (as dev).

Please note: Usage of 5.4 short array syntax in tests! Consequence:
The tests require PHP 5.4. I'm not sure about the compatibility
strategy of phake with regard to PHP versions, but I just wanted to
give it a go in tests. If strict 5.3 compliance is desired, let me know.

Feel free to review and comment. Thank you.

ilkerde added some commits Aug 28, 2013
@ilkerde ilkerde Add unit tests for existing runfile recognition.
In order to extend the recognition of runfiles, tests need to be added
before any code change. PHPUnit and vfsStream have been chosen as tools
for this task and added as dev-requirement through composer.

The existing runfile name tests are located in test/phake/test folder,
which is written into composers PSR-0-compliant autoloader section.

PHPUnit configuration is straightforward and just bootstraps composers
autoloader.
41e2350
@ilkerde ilkerde Support lowercased phakefile filenames.
Support lowercased runfiles to be recognized as well. Example:

    phakefile
    phakefile.php

Tests for proper file recognition added aside.
2ad4937
@ilkerde ilkerde Merge upstream master. (Tests & Reorganization). 9049b08
@ilkerde ilkerde Support lowercase phakefile file names (merged).
Merge efforts for tests from upstream laongside with the lowercase
runfile recognition feature and their respective tests. The tests folder
was changed in favor of upstream definition.

PHPUnits bootstrap now uses the autoloader in order to benefit from
autoloading for test classes as well. The composer.json file reflects
PHPUnit and vfsStream dependecies (as dev).
1915f47
@clue
Collaborator
clue commented Nov 16, 2013

Thanks for this PR!

Would you care to elaborate on the rationale to support lowercase filenames and why sticking to the uppercase one is not an option in your case?

The provided unit tests are very much appreciated. The new developer dependencies certainly introduce some additional complexity, but I'd love to hear some more opinions on that matter (ping @jaz303). Also, I think we shouldn't drop support for PHP 5.3 unless there's a compelling reason to.

Otherwise, the changes look good to me, thanks again!

@jaz303
Owner
jaz303 commented Nov 16, 2013

Re: the feature itself, happy to include this. The capitalisation is a throwback to Rake/Make and not really followed anywhere else.

Tests are a good thing and this PR seems to lay a good foundation for increased test coverage in the future. I haven't done any serious PHP development in ~2 years so I'm a bit out of touch with how projects are organised but if the approach outlined here reflects current best practices I don't see any reason not to merge.

It should be possible to run the tests wherever deployed, so in my mind to say the tests are incompatible with 5.3 is to say the whole project is incompatible with 5.3. That said, 5.3 was EOL'd around 9 months ago so I'm not precious about keeping compatibility - if dropping it stands to improve other aspects of the project.

@ilkerde
ilkerde commented Nov 24, 2013

Thanks for considering this PR.

@clue, the reason for lowercase filename support is simply the expectation that the lowercase named version is being recognized as well. This in fact is a reference to the sibling (or motivating) projects Rake/Make. Both rake and make support lowercase rakefile / makefile. I was just expecting phake to behave as rake does on that.

Regarding 5.3 compatibility, I pretty much second the view of @jaz303. If 5.3 is going to be supported, then the tests should adhere to that. On the flip side, 5.3 really rarely can be found on serious hosters anymore. Nowadays, 5.4 is like the minimum to host any PHP project. An argument for 5.4 is terse syntax and the builtin webserver in CLI. This especially makes unit testing a lot easier.

After all, it's your decision to either drop 5.3 or move to 5.4 support. If you were to stick on 5.3, I can happily replace the 5.4-specific stuff with more traditional statements.

Looking forward for your feedback. Hope it helps!

@clue
Collaborator
clue commented Dec 9, 2013

Both rake and make support lowercase rakefile / makefile. I was just expecting phake to behave as rake does on that.

Totally makes sense to me. So yeah, I'm all for adding this feature :+1:

Perhaps we should consider in which order we look for those files, now that we have a total 4 different file name conventions.

  1. Phakefile
  2. Phakefile.php
  3. phakefile
  4. phakefile.php

Nowadays, 5.4 is like the minimum to host any PHP project

I beg to differ. Personally, I'm using PHP 5.5 and am enjoying its new features (the embedded webserver being one of them).

However, PHP 5.4+ has yet to see major adoption and I'm a bit uneasy with forcing users to upgrade just because using the short array syntax ([1]) is easier to type than array(1). PHP 5.3 still receives support until June 2014 and is still shipped with the latest Ubuntu LTS release (12.04). See also http://w3techs.com/technologies/details/pl-php/5/all and http://dump.seld.be/phpversions.html for some stats. Besides, all of the referenced require-dev's list PHP 5.3.3 as a minimum.

Please don't get me wrong, I'm not saying we should never move. It absolutely makes sense to upgrade once we either require newer features (which frankly I currently don't see being the case) or our minimum requirement starting to become a burden.

My vote:
+1 on supporting lowercased filenames
-1 on introducing side effects (such as the version requirement)

Ultimately, I'm not the one to decide, I'm just raising my concerns :) Perhaps it's easier to separate this discussion to another ticket so we can just merge the new feature?

@jaz303
Owner
jaz303 commented Dec 9, 2013

I think that settles it - if PHP 5.3 is still supported and widely deployed we must continue to support it.

@ilkerde
ilkerde commented Dec 15, 2013

Thanks @clue and @jaz303 for your valuable feedback. I second all your considerations and hence just made sure I'm not using the [] array shorthand from 5.4 anymore.

@clue, as for the order of the filenames to be recognized, the implementation just reflects the order you wrote down. Hope this fits for you.

@clue
Collaborator
clue commented Dec 18, 2013

Thanks for your consideration @ilkerde!

The feature looks good to me, I've finally had the time to test it locally and it works as expected :+1:

One thing that bothers me though is that while previously running phpunit was sufficient, you now have to run composer install to install the dev-dependencies and then have to run ./vendor/bin/composer. This is particularly annoying when regularly switching branches and having to re-install dependencies for each branch.

Do you happen to have any insight on whether there's any consensus on installing phpunit (etc.) via dependencies? I've scanned quite a few projects and they tend towards relying on a system-wide installation of phpunit instead. But I'm very open for any reasons either way.

@ilkerde
ilkerde commented Jan 5, 2014

Hi @clue, thanks for your testing and feedback, hope we can use the feature soon in phake.

I wholeheartedly understand your concerns regarding ./vendor/bin installs. This is a very common concern, so be safe to not feel like a lonesome cowboy with your thoughts. I want to share my experience with this "question". Maybe it helps you to get a solid opinion as well.

I'm dealing with hundreds of repositories, of which dozens use PHP. All those repos have their own dependencies, mostly locking down specific versions of their own dependencies. That's why I install stuff locally - except the package manager itself. It helps me alot having a sane environment for all those projects. (By the way, I install phake to ./vendor/bin as well).

Interestingly, I never had the annoyances you are reporting with having to reinstall packages when switching branches - although I'm a fair heavy user. Maybe it's because I always .gitignore the vendor, as I did it here as well. I never happened to have reinstall issues then, since my local ./vendor is kept. Here's an example with the phake repository itself:

[ilker@x phake]$ git s
## master...origin/master
[ilker@x phake]$ composer show -i
mikey179/vfsStream           v1.2.0 
phpunit/php-code-coverage    1.2.12 Library that provides collection, processing, and rendering functionality for PHP code coverage information.
phpunit/php-file-iterator    1.3.3  FilterIterator implementation that filters files based on a list of suffixes.
phpunit/php-text-template    1.1.4  Simple template engine.
phpunit/php-timer            1.0.5  Utility class for timing
phpunit/php-token-stream     1.2.0  Wrapper around PHP's tokenizer extension.
phpunit/phpunit              3.7.24 The PHP Unit Testing framework.
phpunit/phpunit-mock-objects 1.2.3  Mock Object library for PHPUnit
symfony/yaml                 v2.3.4 Symfony Yaml Component
[ilker@x phake]$ git co upstreammaster
Switched to branch 'upstreammaster'
Your branch is up-to-date with 'upstream/master'.
[ilker@x phake]$ git s
## upstreammaster...upstream/master
?? vendor/
[ilker@x phake]$ composer show -i
mikey179/vfsStream           v1.2.0 
phpunit/php-code-coverage    1.2.12 Library that provides collection, processing, and rendering functionality for PHP code coverage information.
phpunit/php-file-iterator    1.3.3  FilterIterator implementation that filters files based on a list of suffixes.
phpunit/php-text-template    1.1.4  Simple template engine.
phpunit/php-timer            1.0.5  Utility class for timing
phpunit/php-token-stream     1.2.0  Wrapper around PHP's tokenizer extension.
phpunit/phpunit              3.7.24 The PHP Unit Testing framework.
phpunit/phpunit-mock-objects 1.2.3  Mock Object library for PHPUnit
symfony/yaml                 v2.3.4 Symfony Yaml Component
[ilker@x phake]$ 

Sure, from time to time there is some sort of annoyance between branches, especially when specific branches update dependencies while others aren't refreshed - but this is even more a big issue when they are installed globally, ultimately rendering entire projects to be unusable.

In python land, we have things like virtualenv helping us to mitigate these issues. However, I always felt ok with the local package install option of composer.

In order to not always be obliged to type in ./vendor/bin/x but directly invoke the tool in question, I happen to be happy with a short bash alias which expands my PATH when I'm on that particular project. I mostly felt comfortable with this as well.

Now that's about my view and experiences regarding local tool installs. That being said, please let me emphasize clearly that I really don't care whether your preference is to have phpunit package installed locally or globally. I honestly feel fine with both. Consequently, if you should come to the conclusion to rather prefer phpunit installed globally, then please do. It's basically nothing more than removing a single line.

I just wanted to contribute in making phake behave like rake or make. Just because I was used to this behavior. I hope this is ok for you. Thanks for your efforts!

@clue
Collaborator
clue commented Feb 1, 2014

Thanks for taking the time to write such an thorough explanation @ilkerde.

I wholeheartedly understand your concerns regarding ./vendor/bin installs

After some further consideration and some additional tests, I wholeheartedly understand and support your wish to include the development tools.

I've just had the time to check this behavior with several other libraries and it turns out my concerns were mostly untenable. I was under the assumption that running a system wide bin would not work with in conjunction with other local dependencies (such as the vfsStream) on it. As it turn out, they work just fine with a local as well as a global installation, so you can run both just phpunit and ./vendor/bin/phpunit (though admittedly, there are some good reasons to prefer the latter).

I just wanted to contribute

I fully support your reasoning, so sorry for the confusion! Would you care to resolve the remaining merge conflicts? Given @jaz303 already stated his support, I'm looking forward to merge this then, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.