CRM-17652 - Update composer and Symfony for Drupal 8 #10694

dsnopek · 2017-07-18T12:52:16Z

Work in progress PR for https://issues.civicrm.org/jira/browse/CRM-17652

Remaining tasks

Require civicrm/civicrm-packages as a normal composer package

civicrm-builder · 2017-07-18T12:52:17Z

Can one of the admins verify this patch?

totten · 2017-07-18T16:08:20Z

jenkins, ok to test

totten · 2017-07-18T20:00:57Z

composer.json

+  "name": "civicrm/civicrm-core",
+  "description": "Open source constituent relationship management for non-profits, NGOs and advocacy organizations.",
+  "type": "library",
+  "authors": [


Thanks for drafting this. TBH, though, there've been a lot of contributors, and there's a more complete list in CONTRIBUTORS.txt, contributor-key.yml, and release-notes/*.md. I'd like to avoid a fourth one. ;) For composer.json, maybe just list "CiviCRM LLC" or "civicrm.org ". (If a it really needs human, then maybe the current core team.)

It's intended to be humans per the composer documentation, but it's optional and we could even omit it, but pretty much all libraries whose composer.json files I've looked at provide it.

Looking at Symfony's composer.json, they actually put the project lead and then an entry for the "Symfony Community". That's not quite right per the docs, but we could do something similar here? If that's OK with you, I can update my PR!

I like that idea perhaps having the Core team + community, i think the idea is these are the people actively maintaining the thing

mglaman

Thanks @dsnopek! I was hoping to jam on this earlier, but took advantage of my vacation to vacation, instead :P

mglaman · 2017-07-20T16:20:29Z

CRM/Core/ClassLoader.php

@@ -104,7 +104,8 @@ public function register($prepend = FALSE) {
    }
    $civicrm_base_path = dirname(dirname(__DIR__));

-    require_once dirname(dirname(__DIR__)) . '/vendor/autoload.php';
+    // @todo: need some way to check if we should register vendor or not


It's possible to check if a required class exists, and if it does not then load the autoloader?

We could check if \Composer\Autoload\ClassLoader exists. In the generated autoloader_real.php the randomly named class has

public static function loadClassLoader($class) { if ('Composer\Autoload\ClassLoader' === $class) { require __DIR__ . '/ClassLoader.php'; } }

So if that exists, we know an autoloader has been bootstrapped.

Sure, we could do class_exists() on a class from a CiviCRM dependency. I think I'd prefer something more direct, though, like checking if the vendor directory exists, which it shouldn't if installed via composer as opposed to from a tarball... Lemme try that!

Just pushed a new commit that checks for the local vendor autoload, and loads it if it's there

mglaman · 2017-07-20T16:20:37Z

CRM/Core/ClassLoader.php

@@ -123,7 +124,8 @@ public function register($prepend = FALSE) {
    );
    $include_paths = implode(PATH_SEPARATOR, $include_paths);
    set_include_path($include_paths . PATH_SEPARATOR . get_include_path());
-    require_once "$civicrm_base_path/vendor/autoload.php";
+    // @todo: need some way to check if we should register vendor or not
+    //require_once "$civicrm_base_path/vendor/autoload.php";


mglaman · 2017-07-20T16:21:46Z

composer.json

-    "symfony/process": "~2.5.0",
-    "psr/log": "1.0.0",
-    "symfony/finder": "~2.5.0",
+    "symfony/config": "^2.5.0",


We can keep ~ but use || to allow different? so ~2.5 || ~2.8 to compatibility with Drupal. But also, as stated in StackExchange.. 2.5 is outdated and not receiving releases.

Well ~2.5 would allow 2.8 because there isn't the last .0 on there. We probably actually should do ~2.5 || ~3.0 to be compatible with Drupal 8.4.x but I haven't tested with 8.4.x yet

ah, yeah. https://semver.mwl.be/#?package=symfony%2Fprocess&version=~2.5%20%7C%7C%20~3.0&minimum-stability=stable that looks like it'd be the trick (for also supporting 8.4.x)

Whoa, that link is amazing! Thanks for sharing that :-)

totten · 2017-07-20T19:29:41Z

CRM/Core/ClassLoader.php


-    // @todo: need some way to check if we should register vendor or not
-    //require_once dirname(dirname(__DIR__)) . '/vendor/autoload.php';
+    if (file_exists($composer_autoload)) {


The conditional is a good idea, although I'm wondering if we might actually need to check both locations like https://github.com/civicrm/cv/blob/v0.1.27/bin/cv#L9

Reason: for better or worse, Civi has two bootstrap processes:

CMS-first (typical case; ex: a page-request hits Drupal, which then boots Civi by loading civicrm.settings.php and CRM_Core_Config::singleton())

Civi-first (less typical case; ex: request hits extern/rest.php, extern/ipn.php, bin/cli.php, or cv; this reads civicrm.settings.php and then boots CMS via CRM_Utils_System_*::loadBootStrap())

In the second scenario, the classloader needs to be available.

Speculation: this should be fine as long as Civi+CMS behavior are convergent -- they both say require_once vendor/autoload.php for the same copy of autoload.php.

Hm. So, the code should probably be changed to look first for the local vendor directory first (will be present in a non-composer based install), but if it's not there, then look for a top-level vendor directory (will be present in a composer-based install).

I'll take a look! I first need to figure out a good way to test (trying bin/cli.php seems like it'd be the easiest?) and see the failure when using a Civi-first entry point, and then I can try that code change and see if it fixes it.

Thanks for the info :-)

I just pushed some changes that get bin/cli.php bootstrapping Drupal 8 (or at least not crashing when trying to do so). It required creating a settings_location.php file because the default logic for finding the Drupal config directory in civicrm.config.php doesn't work with civicrm-core installed by composer. I don't know if there's a sane way to fix that, since I don't know what all the cases in there are for and I don't want to break something

dsnopek · 2017-07-21T15:53:55Z

I finally got around to testing with Drupal 8.4.x -- the composer change described above in the thread with @mglaman works!

However, there were some additional changes required, because there was some stuff added in Symfony 2.6 which deprecated some Symfony 2.5 methods, and of course, the deprecated methods are removed in 3.x. So, I had to change some code around building the service container, and push the minimum Symfony dependency up to 2.6. Hopefully, that's acceptable? If not, I can make a separate branch/PR for supporting Drupal 8.4, but 8.4 will be release on October 4th, so it's coming up pretty quick

But after all that, I got it working on Drupal 8.4 :-)

totten · 2017-07-21T18:40:02Z

@dsnopek nice work

Regarding the version bump to 2.6, that works for me. Two caveats:

I was a little worried about future developers running composer update (with the intention of updating one package but actually updating all), and that would be hard to spot, so CRM-17652 - composer.lock - Add unit test to redflag policy change dsnopek/civicrm-core#1
An extension like this could have stale references to setFactory(Class|Service|Method)... but it's not a widely known technique in our community, and that was the only example I could find in the universe of published extensions, and that extension tends to follow an evergreen attitude ("you should always be on latest 4.7.x"). So carry on. :)

dsnopek · 2017-07-21T19:01:07Z

An extension like this could have stale references to setFactory(Class|Service|Method)...

Ah, yeah. Well, that'll continue to work in any Symfony 2.x version (those methods are still there but deprecated), but that'll fail when using that extension with Symfony 3.x, ie. with Drupal 8.4. So, that extension won't break right away, and maybe the developer will pay attention to the deprecation notices :-)

seamuslee001 · 2017-07-22T00:20:25Z

@dsnopek David i just want to check that you were able to enable other modules through the GUI once CiviCRM had been installed is that correct?

dsnopek · 2017-07-22T01:28:09Z

@seamuslee001 Yes! I reported that worked on this comment on the JIRA ticket - I've written some crazy large comments on that issue, so I don't blame you for not noticing that bit of information :-)

jackrabbithanna · 2017-07-22T16:48:34Z

How awesome is this! Fantastic @dsnopek really appreciate your time with this...So how do I test this...Do I just apply the patch to an already working installation, or do I need to use civibuild to build CiviCRM against master + this PR ? Are their any steps for applying this for testing that we should be aware of?

dsnopek · 2017-07-23T12:06:10Z

It's the same process as I originally posted in JIRA and on Stack Exchange (but I have been evolving the gist which is the steps to replace composer require civicrm/civicrm-core until all this is committed and the issues around assets and what's committed in the repo are fixed).

Here's the steps again:

Download and install Drupal 8.3.5 (or the latest dev of Drupal 8.4.x!)

Go into the root directory in the shell and run these commands to install CiviCRM via composer (one day this will just be composer require civicrm/civicrm-core): https://gist.github.com/dsnopek/56311dbea347874e75180883efabb620

If you use Apache, remove the vendor/.htacess file. This is a security measure from Drupal, which prevents resources like CSS/JS being loaded. This will need some collaboration with the Drupal project to figure out a proper solution for because removing this file altogether is a bad idea on production. See: https://www.drupal.org/node/2896308

Go into the /modules directory and do git clone https://github.com/dsnopek/civicrm-drupal.git --branch composer-library

Go to the "Extend" page (/admin/modules and install the CiviCRM module

Clear drupal cache via Drush (drush cr

Log out and log back in again per CRM-19878

CiviCRM works!

I don't know if anyone other than me has tested these steps yet, so I'd really appreciate any feedback on them!

dsnopek · 2017-07-25T15:41:33Z

Hrm. I merged the master branch (because there was a conflict) and it's failing in Jenkins in a way I don't totally understand. Any advice on how to fix it?

seamuslee001 · 2017-08-03T04:43:45Z

Hi @dsnopek I would recommend that you rebase your branch against civicrm's master branch with git rebase -i /master then do git push composer-library -f

dsnopek · 2017-08-03T17:08:26Z

@seamuslee001 Hm, I'd normally not rebase on a branch that was pushed remotely. However, if that the normal process for CiviCRM PRs, I can certainly do that!

…y 1.0.*)

…classes

…CiviCRM classes" This reverts commit 51d83d0.

…tible with Symfony 3.x (need new method to replace deprecated method)

dsnopek · 2017-08-03T17:18:29Z

Rebased! Hopefully tests will start working again :-)

dsnopek · 2017-08-03T19:20:21Z

And we're back to passing. Yay! :-)

KarinG · 2017-08-05T19:59:45Z

Hi @dsnopek - thanks for all your work on this! I went through the steps you listed - above - and 💯 x 👍 !

I've got a functional (freshly baked) Drupal 8.3.6; CiviCRM 4.7.22; I've been able to:

install Drupal Telephone module and add a Telephone field to a Content Type (all via the GUI)
install our iATS Payments Extension via the CiviCRM Extension interface: drupal8.local/civicrm/admin/extensions?reset=1 (Add New)
process a LIVE (fake VISA) payment w/ iATS (in CiviCRM)
install webform module and webform bootstrap module
submit a webform

Thank you & Merci!

seamuslee001 · 2017-08-05T21:33:01Z

@totten i feel like with Karin's comment this has had plenty of testing now and seems sane

KarinG · 2017-08-05T22:38:20Z

Just to clarify - I followed the exact steps in: #10694 (comment)

xurizaemon · 2017-08-06T00:14:06Z

CRM/Core/ClassLoader.php

+   * @return void
+   */
+  protected function requireComposerAutoload() {
+    $civicrm_base_path = dirname(dirname(__DIR__));


Pretty sure you can use

$civicrm_base_path = dirname(__FILE__, 2); $top_path = dirname(__FILE__, 5);

Would be good to include a comment clarifying what's magic about the 5-th parent directory (and/or use a Drupal method to discover Drupal root, if that's what that is).

That's neat.

According to http://php.net/dirname , the second argument was added in PHP 7. We'll be stuck with multiple dirname()s for a while.

dsnopek · 2017-08-06T13:49:34Z

Thanks, @KarinG! I'm super glad it worked :-)

@xurizaemon Whoa! How is it possible that I never knew that dirname() had a 2nd argument? Thanks for the tip. I'll update the code and add some comments as suggested (probably tomorrow).

I don't think we can use a Drupal method to discover the Drupal root because we could end up here through an alternate entrypoint before Drupal is bootstrapped (ex. cli.php like @totten describes in this comment) and, at some point in the future, some other CMS could be composer-based and this same code would still need to find the top-level vendor directory which will be in the same place relatively, so this way is more generic.

dsnopek · 2017-08-07T14:12:14Z

I just pushed a commit with the changes suggested by @xurizaemon!

dsnopek · 2017-08-08T21:06:50Z

Hrm. It looks like the tests are failing because the version of PHP that's running the tests doesn't support the 2nd argument to dirname() - so, I'll have to revert that!

…by Jenkins

totten · 2017-08-12T04:53:50Z

I think these changes are better in than out. @dsnopek , do you still want the "WIP" label on here?

Aside: In the past, we've encountered unexpected issues when touching this area (esp. involving tarball builds), so I'd like to do a quick trial-run of generating+installing tarballs before merging. So far, civihydra passes for D7+WP. Having a couple issues with latest Backdrop+Joomla, but I think they're tangential. Will post again when I've resolved those.

jackrabbithanna · 2017-08-12T04:55:35Z

I tested David's process and it works out. You must install Drupal the normal way. In stalling Drupal with composer breaks loading any css and js resources because when you install Drupal via composer you can't set the resource url because the webroot is in the web directory of the drupal docroot, and vendor directory can't be set for the resource url.

dsnopek · 2017-08-12T05:13:20Z

I think these changes are better in than out. @dsnopek , do you still want the "WIP" label on here?

@totten Like I said on the JIRA issue, I think this is mergable in it's current state. There's still more that should be done to improve the installation process, but we can address that in follow-ups. So... I'll remove the "WIP" label!

jackrabbithanna · 2017-08-12T05:17:17Z

Yep we have two separate verification tests, it's definitely a huge improvement for the issue. It's got my vote

totten · 2017-08-12T07:29:29Z

OK, I've confirmed that my testing issues on Backdrop+Joomla were unrelated.

Let's merge this!

dsnopek · 2017-08-12T12:11:42Z

Woohoo! Thanks, Everyone! :-)

KarinG · 2017-08-12T15:22:11Z

Jippie! What's next? It would be aswesome if we can civibuild create dmaster --cms-ver 8.3.6 ahead of the upcoming Sprints @totten!

totten · 2017-08-14T19:03:09Z

@KarinG IIRC, it has been possible to say civibuild create d8master or civibuild create mybuild --type drupal8-demo --cms-ver 8.3.6 for quite a while. That calls the download.sh and install.sh from drupal8-demo.

Those scripts need to be updated, though. They match the original file-layout of the D8 port. Now that we're making progress toward composer-based distribution, the scripts should instead use composer for setup.

KarinG · 2017-08-14T19:57:44Z

Hi @totten - sorry - yes that's what I meant - would you be able to update those scripts by the time the Sprints come around? I got things to work following the steps outlined above but it's a lot of steps/long process; it would be awesome if you could update your buildkit scipts.

konadave · 2017-08-15T17:43:32Z

I was able to install into a drupal-composer/drupal-project based site. Follow the instructions above from the project root, not Drupal root. Then...

cd web
ln -s ../vendor
chmod 0775 sites/default

That got it to install, though it was broken; no menu, no style, etc. Go to http://www.yourdomain.com/civicrm/admin/setting/url?reset=1 and set the first edit control (there's no label displayed) to http://www.yourdomain.coml/vendor/civicrm/civicrm-core and save.

I haven't done extensive testing but so far it seems to be working fine and I'm able to install other modules.

jackrabbithanna · 2017-08-15T17:49:27Z

@konadave so for it to work currently, you want to install Drupal 8 "like normal", that is not with composer....then follow the steps that David S. has provided.

the reason that the css, js etc... doesn't work, is because with a standard composer based install, the website doc root is in <project_root>/web ... basically the website docroot and the project root are not the same directory. CiviCRM is then placed in <project_root>/vendor/..... and you can't set the Resource URL to point to the Civi installation, because it is out of the website docroot....

Further work will be required in CiviCRM Core to make this work with D8 installed in the beginning with composer

konadave · 2017-08-15T17:52:51Z

@jackrabbithanna I have it working using the steps I outlined. The symlink puts the vendor directory in docroot.

jackrabbithanna · 2017-08-15T17:53:29Z

@konadave sorry I guess I didn't read your comment well the first time....thanks for documenting that...Not sure if that should become the "set" method or if something else should be developed, if so we should make another jira issue and pr about that

civicrm-builder added the master label Jul 18, 2017

totten reviewed Jul 18, 2017

View reviewed changes

mglaman reviewed Jul 20, 2017

View reviewed changes

totten reviewed Jul 20, 2017

View reviewed changes

dsnopek mentioned this pull request Jul 25, 2017

CRM-17652: Make civicrm-packages into a composer library civicrm/civicrm-packages#193

Merged

dsnopek added 13 commits August 3, 2017 12:09

Add additional information to composer.json to make it into a library

52a05d3

Allow patch version updates of psr/log

ae7c4b1

Allow Symfony versions up to 3.0.0 (and fix psr/log constraint to onl…

6be3826

…y 1.0.*)

Attempt to avoid using autoloader if we can already autoload CiviCRM …

9aa662d

…classes

Revert "Attempt to avoid using autoloader if we can already autoload …

9eb18cc

…CiviCRM classes" This reverts commit 51d83d0.

Don't load the vendor autoload

2a6574d

Add 'license' key

be652e9

Update to civicrm-cxn-rpc with less strict psr/log dependency

5f2a990

Switch 'authors' to CiviCRM core team and community

8fc5711

Only load autoload.php from local vendor directory if it exists

7991d8e

Try Symfony 3.x for Drupal 8.4.x

a073544

Update to Symfony 2.6 as lowest supported version, so we can be compa…

a892989

…tible with Symfony 3.x (need new method to replace deprecated method)

Switch back to old PHP array syntax

d15f7e1

dsnopek and others added 2 commits August 3, 2017 12:17

Oops! Bad conversion of Symfony 2.5 to 2.6 code

5eda8e9

CRM-17652 - composer.lock - Add unit test to redflag policy change

e73a88e

dsnopek force-pushed the composer-library branch from 3dcf001 to e73a88e Compare August 3, 2017 17:17

xurizaemon reviewed Aug 6, 2017

View reviewed changes

Use 2nd argument of dirname() and add a comment explaining paths.

d1f6065

Revert usage of dirname()'s 2nd argument to support PHP version used …

9f1588f

…by Jenkins

dsnopek changed the title ~~(WIP) CRM-17652 - Update composer and Symfony for Drupal 8~~ CRM-17652 - Update composer and Symfony for Drupal 8 Aug 12, 2017

totten merged commit a1c16b9 into civicrm:master Aug 12, 2017

dsnopek mentioned this pull request Aug 12, 2017

CRM-17652: Symfony conflict between Civi and Drupal 8 civicrm/civicrm-drupal#465

Merged

CRM-17652 - Update composer and Symfony for Drupal 8 #10694

CRM-17652 - Update composer and Symfony for Drupal 8 #10694

Conversation

dsnopek commented Jul 18, 2017 • edited Loading

Remaining tasks

civicrm-builder commented Jul 18, 2017

totten commented Jul 18, 2017

Choose a reason for hiding this comment

dsnopek Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mglaman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsnopek commented Jul 21, 2017

totten commented Jul 21, 2017

dsnopek commented Jul 21, 2017

seamuslee001 commented Jul 22, 2017

dsnopek commented Jul 22, 2017

jackrabbithanna commented Jul 22, 2017

dsnopek commented Jul 23, 2017 • edited Loading

dsnopek commented Jul 25, 2017

seamuslee001 commented Aug 3, 2017

dsnopek commented Aug 3, 2017

dsnopek commented Aug 3, 2017

dsnopek commented Aug 3, 2017

KarinG commented Aug 5, 2017 • edited Loading

seamuslee001 commented Aug 5, 2017

KarinG commented Aug 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsnopek commented Aug 6, 2017

dsnopek commented Aug 7, 2017

dsnopek commented Aug 8, 2017

totten commented Aug 12, 2017

jackrabbithanna commented Aug 12, 2017 via email

dsnopek commented Aug 12, 2017

jackrabbithanna commented Aug 12, 2017

totten commented Aug 12, 2017

dsnopek commented Aug 12, 2017

KarinG commented Aug 12, 2017

totten commented Aug 14, 2017

KarinG commented Aug 14, 2017

konadave commented Aug 15, 2017

jackrabbithanna commented Aug 15, 2017

konadave commented Aug 15, 2017

jackrabbithanna commented Aug 15, 2017

dsnopek commented Jul 18, 2017 •

edited

Loading

dsnopek Jul 19, 2017 •

edited

Loading

dsnopek commented Jul 23, 2017 •

edited

Loading

KarinG commented Aug 5, 2017 •

edited

Loading