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

CRM-21093: Move CiviCRM initialization from service constructor to method. #495

Merged
merged 3 commits into from
Oct 9, 2017

Conversation

dsnopek
Copy link

@dsnopek dsnopek commented Sep 26, 2017

@jackrabbithanna
Copy link
Contributor

The PR looks good to me, but it failed the tests....Although when I look into why it failed, the console output was "Build step 'Publish xUnit test result report' changed build result to FAILURE"

@bgm or @totten would you'll have an idea what that means?

@jackrabbithanna
Copy link
Contributor

Other than that I think that this should be merged

@seamuslee001
Copy link
Contributor

I believe this looks ok to me but haven't tested @jackrabbithanna re tests, Tests are currently not enabled for Drupal 8 because the PR unit test box is php5.3 not php5.4 (as a min). In the console output it has "PR test not allowed for 8.x-master"

@jackrabbithanna jackrabbithanna self-requested a review October 6, 2017 19:23
@jackrabbithanna
Copy link
Contributor

jackrabbithanna commented Oct 8, 2017

Ok it looks good so far, but there is one more place you need to fix.

I'm gettnig PHP Fatal error: Class 'CRM_Core_BAO_UFGroup' not found in /var/www/html/drupal83/modules/civicrm-drupal/src/Form/UserProfile.php on line 91

Going to the user view page, and a error on the user edit page...

need to add
\Drupal::service('civicrm')->initialize();

To the UserProfile class, in src/Form/UserProfile.php

@jackrabbithanna
Copy link
Contributor

I also think you will need to add it to the __construct() of the CivicrmContoller Class in src/Controller/CiviCRMController.php

@jackrabbithanna
Copy link
Contributor

jackrabbithanna commented Oct 8, 2017

I'm getting this error, when clearing Drupal caches:
Call to undefined function civicrm_api3_create_error() in /var/www/html/drupal83/vendor/civicrm/civicrm-core/Civi/API/Kernel.php on line 413

Was this fixed in another issue, and my test instance is behind?

Every Drupal page will WSOD after that, until I go to /civicrm

@jackrabbithanna
Copy link
Contributor

I remember now, the cache clear WSOD is caused by civicrm_views, which you were fixing here:

https://github.com/civicrm/civicrm-drupal/pull/466/files

Perhaps we should add the require statement below:
require_once 'api/class.api.php';

To the initialize() function of the service, and then instantiate the service and run the initialize() method on it in PR #466

@dsnopek
Copy link
Author

dsnopek commented Oct 9, 2017

I'm gettnig PHP Fatal error: Class 'CRM_Core_BAO_UFGroup' not found in /var/www/html/drupal83/modules/civicrm-drupal/src/Form/UserProfile.php on line 91

Oops! Thanks, I missed one. :-) I searched for other instances for the comment about not doing anything with the Civicrm service and found one place I forgot to clean it up, but not more needed functional changes.

I've pushed a couple new commits for those things!

I also think you will need to add it to the __construct() of the CivicrmContoller Class in src/Controller/CiviCRMController.php

I don't think so, actually, because $civicrm->invoke() will automatically initialize the service if it already hasn't been.

I remember now, the cache clear WSOD is caused by civicrm_views, which you were fixing here #466

The fix in #466 doesn't actually work yet :-( Let's get this one merged (once it all looks good, of course!) and figure that one out on that PR

@jackrabbithanna
Copy link
Contributor

Ok, @totten, @eileenmcnaughton @colemanw I've tested this PR and reviewed the code. It is all good from my perspective, and other PRs are depending on this one to move forward. I'm requesting this get merged.

@colemanw colemanw merged commit 4a4d5af into civicrm:8.x-master Oct 9, 2017
@dsnopek
Copy link
Author

dsnopek commented Oct 9, 2017

Woohoo, thanks! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants