Using trait without PHP 5.4 requirement breaks Drupal CI of all modules that require UUID

The release notes for uuid 7.x-1.0-beta2 state "Switch to trait for assertUuid and bump minimum PHP version to 5.5". It looks like the PHP version change didn't happen.

See #2763727-8: Optimizations and fixes for tests for an explanation of this change. Closing as WONTFIX.

Reopening because a few days ago CTools released a new version 7.x-1.13 which adds uuid as a test dependency.

Now ALL MODULES which depend on CTools cannot be tested with PHP 5.3 because of what the uuid module is doing.

CTools has about 1 million users, and hundreds of modules depend on CTools (e.g. Views). It makes far more sense to fix this in uuid rather than force all those other modules to implement workarounds for your choice.

In that other issue you say

We agreed that adding php = 5.4 could break sites for users stuck on Ubuntu 12.04LTS and other distros using PHP 5.3. The trait is only used in the tests.

But if you require PHP 5.4, which you do, then not declaring that in your .info DOES break automated testing for every project which uses CTools. And we have to be able to test against 5.3 to ensure that things like short array syntax don't slip into our codebase and cause fails for all those sites you're concerned about. Indeed, this is exactly what happened - CTools stopped testing against 5.3 because of the uuid module and then a short array syntax did get committed and now this affects hundreds of thousands of sites.

So please reconsider your decision. Your use of a trait is not essential - it's just a convenience and you can easily modify your tests to not use the trait. I'll write the patch if you really want.

I'm marking this critical because my module, like many others, can no longer be tested against PHP 5.3 and testing is vital when maintaining several hundred thousand lines of code deployed on tens of thousands of sites.

Attached is a patch which removed the trait - it actually saves 1 line of code ...

I'm posting this and testing against PHP 5.3 to see if I can identify any other PHP 5.3 incompatibilities ...

Nice work @TR, it would be good to keep this at the lowest dependency that it really needs.

EDIT: not sure how #4 comment relates to ctools, that may be a rant from the other issue. UUID isn't a dependency of CTools.

CTools 7.x-1.13 introduced a test dependency on uuid, and uuid module functions are used in the CTools tests as an alternative way of generating uuids (alternative to the built-in CTools uuid generation).

Also apparently the uuid_services_example module depends on CTools, so we have a co-dependency here where the PHP 5.3 tests in #5 won't run because CTools 7.x-1.13 uses short array syntax in ctools.test and short array syntax isn't allowed in PHP 5.3.

So yes, CTools is intimately related to this issue... This is also mentioned by MegaChriz in #2942581-8: ctools.test breaks Feeds tests on PHP 5.3 because of short array syntax

Ah I see #2827100: Improve test coverage and function docs, what a complicated web we weave... did you want to remove the RTBC status?

I'm looking at reverting parts of that issue and discussing with other ctools co-maintainers. I still think this is RTBC worthy.

I guess I inadvertently removed the RTBC ... let me put that back for now.

This patch IS necessary in order to run with PHP 5.3, but I can't tell if it's sufficient to fix all the PHP 5.3 problems.

To test uuid with 5.3, CTools has to first have a new release - we can't test this patch against 5.3 on the testbot because dependencies are calculated before patches are applied, and the CTools latest release has that pesky short array syntax ... Thus we can't just add a dependency requiring an earlier version of CTools to avoid the syntax error. Besides, uuid doesn't declare an explicit CTools dependency - rather the submodule uuid_services_example has a dependency on Services, and Services has a dependency on CTools which is how CTools is being pulled into this.

So I think we have to do this incrementally. I think the minimum steps are something like this:

1. Fix the trait, commit to uuid-dev.
2. Commit a change to the CTools test_dependency to pick the -dev version of uuid so that CTools tests won't fail under 5.3 because of this.
3. If CTools-dev can then run with PHP 5.3, CTools is ready for a new release. If not, the test_dependency should be removed and a new CTools release made anyway. A new CTools release is a blocker for other modules trying to test on 5.3.
4. If additional PHP 5.3 issues with uuid were identified in the previous step, those will then need to be fixed here. But they can't be identified until there's a new CTools release.
5. After 4) is complete, CTools can add back the test_dependency on uuid-dev until a new uuid release is made.

Alternatively, if the maintainers of uuid are not responsive to this change,

1. Remove the CTools test_dependency on uuid so that this uuid bug won't affect CTools 5.3 testing.
2. Make a new release of CTools with the test_dependency removed (and the already-fixed short array syntax).

we can't test this patch against 5.3 on the testbot because dependencies are calculated before patches are applied,

You can, you just have to do it with a composer.json. The testbots, via composer, is able to detect changes to composer.json, but cannot do so without a commit if the dependency changes are solely in .info.yml

require-dev is the equivalent of test_dependencies as far as the testbots are concerned.

@Mixologic: Does the composer.json need to be committed first or can it be introduced in the patch? And will adding a composer.json here in the uuid module which declares an explicit dependency on CTools -dev override the CTools dependency of the Services module, which doesn't have a composer.json?

@TR composer.json files can be introduced by a patch, and the dependency re-calculation will happen.

@TR missed your edit:

if uuid has a dependency on ctools-dev, then yes, composer will install the ctools dev version unless somewhere else something is explicitly declaring a dependency on 7.x-1.13.

OK, I'll try that. Thanks.

This patch adds a composer.json to the patch in #5, to try to test this patch with PHP 5.3.

@Mixologic: That didn't seem to work - is my composer.json correct in #16?

@TR
The composer.json file looks correct. The console however says "Ignoring Composer Changes" on https://dispatcher.drupalci.org/job/drupal_d7/63641/console.

I compared this to https://dispatcher.drupalci.org/job/drupal_d7/40059/console, as I did add a dev dependency in a composer.json before for a D7 module, though I did it directly on commit: https://cgit.drupalcode.org/uc_extra_fields_pane/commit/?id=e4a24f3a863b...

@TR: Re #17: My apologies as I had misled you.

The feature that detects updated composer.json files in drupalci as part of the build process was only implemented for d8, and there was just a stub waiting to be implemented for d7.

I went ahead and fixed that(#2945467: d7 testing should support patches to composer.json), and deployed it.

*now* the tests are failing because the composer.json is incomplete, because it's missing the other testing dependencies that are scattered throughout the project in .info files.

Fortunately a `composer show -a drupal/uuid`shows us what those should be, so if you add

drupal/rest_server *
drupal/services *
drupal/uuid_services *
drupal/entity *

To the require-dev list in composer.json, the patch should work.

OK, let's try it with the change to DrupalCI and the changes to composer.json mentioned in #19.

Again, this patch adds a composer.json to the patch in #5, to try to test this patch with PHP 5.3.

Again.

I don't know how to specify a version for drupal/uuid_services, which is a submodule of uuid, and is currently being tested. Do I even need this requirement at all since it's a submodule and the code will already be loaded?

Testing uuid 7.x-1.x-dev, I tried:
#20: drupal/uuid_services ~1.0
#22 drupal/uuid_services ~1.x-dev
Both failed.

Let's try without that requirement ...

The patch in #24 demonstrates that when the trait is removed from the uuid module, uuid becomes testable under PHP 5.3.

So to follow-up on #10, the patch (#5) IS necessary and IS sufficient to make uuid tests run under PHP 5.3, and therefore fix the CTools/Views/etc problems with PHP 5.3 tests.

Here's a revised list of what needs to be done:

1. Fix the trait by committing #5 to uuid-dev. Optionally release a new version of uuid with this fix.
2. Commit a change to the CTools test_dependency to pick up the -dev version of uuid so that CTools tests won't fail under 5.3 because of this.
3. CTools is then ready for a new release. A new CTools release is a blocker for other modules trying to test on 5.3.

Alternatively, if the maintainers of uuid are not responsive to this change,

1. Remove the CTools test_dependency on uuid so that this uuid bug won't affect CTools 5.3 testing.
2. Make a new release of CTools with the test_dependency removed (and the already-fixed short array syntax).

Thanks for the work on this @TR

Let's get sign off from the people that have been reviewing the solution proposed here before RTBCing :)

Getting rid of a test trait and replacing it with an abstract class is a good way to solve this, and #24 shows that it works. AFAIC, nothing in this patch can be improved, and will continue to allow other projects to test against 5.3.

Whether or not new development on drupal7 should *still* be backwards compatible with a version of php that was EOL'ed almost 4 years ago is probably best for another discussion...

+1 for patch #24. Tested without issues.

Thanks all for the work on this (and the patience).

Is there a way to do this without a composer.json?

When using composer manager and uuid now tries to get these Drupal packages, but the Drupal composer endpoint is not added by default.

@timmillwood
The composer.json lists the dependencies in "require-dev", so there's no need to retrieve these packages. It's been a while since I last used Composer Manage. But if I remember well, it combines all composer.json files into one single composer.json file? And then you manually need to execute a composer install after, right? If so, then just execute composer install --no-dev would do the trick.

When using composer_manager if you run drush --yes en composer_manager which we have in our automation scripts, it installs the module then does a composer update. This includes dev dependencies.

https://git.drupalcode.org/project/composer_manager/blob/7.x-2.x/compose...