PHP5.6

[dshanske]

Close #104.

This does things a bit differently than @gRegorLove #219 .

• It updates Travis to test for 7.4 and removed 5.4 and 5.5.
• It adds phpcs and phpcompatibility to check that the code can run on the minimum version required
• It adds scripts for phpunit and phpcs into the composer file to ensure the version in the vendor directory is being run, not the global version that might be installed. I kept forgetting this, and I think the latest phpunit is several versions ahead of what we're using.
• It adds instructions for both into the testing in the README

[pfefferle]

👍

[gRegorLove]

I'm still confused by the approach of the config.platform in composer.json. Shouldn't the composer.lock have only dependencies that are compatible with 5.6? Then if someone installs on PHP7 they can run composer update to get PHP7-compatible dependencies. My understanding of the way it is currently is even if you're running PHP7, you're locked in to the 5.6-compatible dependencies. I could be misunderstanding config.platform though.

[pfefferle]

@gRegorLove oh, haven't realized that change. You are right, we should not set the config plattform to php 5.6! Here is a good summary of the param: https://andy-carter.com/blog/composer-php-platform

[dshanske]

So, it has to be set to 5.6, run for composer.lock, then removed.

[gRegorLove]

@dshanske That sounds good.

[pfefferle]

But why has the composer.lock has to be 5.6? If there is a composer.lock, composer uses this file for updates, so it does not help to remove the Plattform from composer.json?!?

[dshanske]

@gRegorLove Should we merge this?

[pfefferle]

@dshanske can you explain why it has to be 5.6 in the .lock version?

If it stays like this, it will break the next time someone updates the composer.* files and/or after updates in local/test environments.

[pfefferle]

should be "10."

[gRegorLove]

I'd also suggest switching 6 and 7 to encourage TDD.

6. Add PHPUnit tests to cover what you want to change, either in an existing test file if suitable, or a new one
7. Make your changes

[dshanske]

@dshanske can you explain why it has to be 5.6 in the .lock version?

If it stays like this, it will break the next time someone updates the composer.* files and/or after updates in local/test environments.

I'm perfectly fine with no longer distributing a composer.lock. But this repo has had one distributed for a while.

[gRegorLove]

I don't know the side effects / benefits of distributing without the composer.lock, so I lean towards keeping it for now.

Since we're supporting PHP5.6 still, my understanding is the composer.lock should only have 5.6-compatible packages. In PHP7 environments, running composer update should upgrade packages to 7-compatible versions.

[pfefferle]

@gRegorLove but does it really work like this? If you run composer update it will update the .lock file in both cases (php5.6 and php7x) so the config.platform will always be removed.

I still vote against an inconsistant state, where the composer.lock file has different infos as the composer.json, because it might confuse people and cause problems.

I still do not understand in which case the config.platform helps. Either the dependencies (PHPUnit) are compatible with 5.6 or not... or am I wrong?

[gRegorLove]

lgtm

[gRegorLove]

@pfefferle Where is the inconsistency? There isn't a config.platform for PHP 5.6 now.

[pfefferle]

@gRegorLove it was only removed from the .json file and is still in the .lock file: https://github.com/microformats/php-mf2/pull/220/files#diff-1da2c7edc898c70e5a79a9997c98ceccR1431

So you might get different results when you run composer install vs composer update.

[Zegnat]

I think this will depend on how @dshanske ran the composer install command? Did you use composer’s platform configuration to force 5.6, or did you go with @gRegorLove’s method of using a php56 binary?

I do not have php56 handy right now, but building with composer on PHP 7.4.4 the platform-overrides key does not get added for me:

jq '.["platform-overrides"]' composer.lock
# {
#   "php": "5.6.1"
# }
rm -rf vendor composer.lock
# 
composer config --list | grep 'platform'
# 
composer config --global --list | grep 'platform'
# 
composer install
# … snip …
jq '.["platform-overrides"]' composer.lock
# null

@dshanske could you make sure that your composer config does not have a platform set when running the install that generates the lock file? (You can borrow the lines above.)

I may have a go at updating this branch if I can succeed with a PHP 5.6 binary. Though if you want the lock file to have any value at all, I guess we need to use specifically PHP 5.6.0? Since dependencies will have changed (a lot?) between running php-mf2 on 5.6 and 7.4, I do wonder what the value of the lock file is in a project like this. Are we expecting people installing php-mf2 to always run with these dependencies, or are we expecting them to do their own composer update before even getting compatibility with their own version of PHP? To me a lock file really only makes sense in the first case, but our own testing seems to be angled at the latter case?

[Zegnat]

Errant thought while looking through some still-open PRs: do we win anything by also bringing #213 in here?

By taking the step to PHP 5.6, I believe we can bring PHPUnit up all the way to version 5 (5.7.27 being the latest of that branch, I believe).

[pfefferle]

I think there are two options, removing the .lock file or removing the dev/test requirements (and load them directly in the CI tool)?!?

[Zegnat]

We may have been going at this wrong. Taking another look at the previously failing Travis logs, like build #246, it seems like that one actually worked. Running composer update instead of composer install made sure every version of PHP fetched their correct dependencies.

The problem was that Travis’ install runs before before_script. So what was happening was that when the test was running with the userland HTML5 parser, it would require this dependency before running composer update. By loading this extra dependency before resolving the other dependencies, Composer proceeded to load everything from the lock file and failed on systems older than the one the lock file originated from.

If someone here with more knowledge of Travis would like to comment, that would be much appreciated, but from a quick skim of Job Lifecycle documentation I feel like we should run both composer update and $COMPOSER_REQUIRE within the install step, and in that specific order.

--- a/.travis.yml
+++ b/.travis.yml
@@ -16,5 +16,5 @@ matrix:
   allow_failures:
     - php: nightly
 install:
+  - composer update
   - $COMPOSER_REQUIRE
-before_script: composer install

I think if we do that, then it should not longer matter what version of the lock file we commit to the repo.

Not confident enough to make this commit myself, as I feel like I have no idea how our current Travis setup works. As an example I took a look at the one used by the HTML5 parser and it seems way more complete than ours: it runs composer self-update and phpunit in there, which seems to be happening automagically for us?!

[Zegnat]

As of my last commit in this branch:

1. Travis will always run composer update to get the dependencies right for the PHP version it is currently testing. All versions tested pass.
2. Because Travis now always fetches dependencies from scratch, it does not matter what version of the Composer lock file we check in to the repository.
3. The lock file currently checked in is the one generated with PHP 7.4.4. This means if developers are running the latest PHP stable, they should be able to just do composer install and have it grab all dependencies listed in there. Hopefully saving them some round trips.

I propose we aim to keep the Composer lock file somewhat in line with what we expect other developers to want to use locally, which I would say is anything that matches current stable PHP. If others are OK with that, I think this branch is now merge-able! 🎉

[gRegorLove]

I think that sounds good, @Zegnat. Do we need to include any specific instructions for anyone installing on PHP5.6 or earlier versions of 7?

[Zegnat]

Do we need to include any specific instructions for anyone installing on PHP5.6 or earlier versions of 7?

That depends whether we think how likely that is to occur.

Note that the lock file only applies when you checkout the repository, maybe because you want to work on the parser as a project, and run composer install. If you are on a lower version of PHP but you use composer require mf2/mf2 to add it as a dependency to whatever your actual own project is, Composer ignores the lock file and will go and get the dependencies that matches your current setup. (Or at least it should, as far as I understand.)

How likely is it someone will checkout this repository and run just the parser, or just the test, on an older platform? And if that person’s goal is to develop for the older platform, how likely is it they need specific instructions to know to run composer update first?

[Zegnat]

@pfefferle GitHub does not let me re-request a review from you, so I am choosing to tag you instead 😄 Would love to get another opinion on this one if you have the time!

[gRegorLove]

use composer require mf2/mf2 to add it as a dependency to whatever your actual own project is, Composer ignores the lock file and will go and get the dependencies that matches your current setup

Aha, I was misunderstanding thinking require would use the lock file, but this makes sense. I haven't tested in PHP5.6 but I think it's mergeable now.

[pfefferle]

@Zegnat sure, I will have a look later today (sadly, Github only allows to add reviewer that have direct access to the repo)

[pfefferle]

I second @gRegorLove, I think it is mergeable now.

I was also not aware, that the .lock file is only important when working on the lib itself. There is an interesting part in the composer doc about that:

For your library you may commit the composer.lock file if you want to. This can help your team to always test against the same dependency versions. However, this lock file will not have any effect on other projects that depend on it. It only has an effect on the main project.

Thanks @Zegnat !

[Zegnat]

@pfefferle thanks for giving this another look! I always find it a little silly that GitHub will not let some request a re-review of someone who actually did leave a review, even when they aren’t a repo owner/contributor.

Lock files are not considered when a library is used as dependency because all different dependencies’ dependencies are resolved together to make sure there are no clashes. This is always done based on the default version strings from the package file. (Same goes for Node’s npm by the way!) That is also why people sometimes argue that you do not need to commit lock files for libraries at all.

I’ll merge this now! 🚀