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-17917 - Multiple fixes for fresh Drupal 8 installation #388

Merged
merged 2 commits into from
Jan 6, 2017

Conversation

totten
Copy link
Member

@totten totten commented Jun 17, 2016

See detailed descriptions in commit messages.


When setting up a new build (e.g.  `civibuild create d8-master`), I
experienced crashes because this code calls a non-static function using a
static syntax.  (Specifically, `FileSystem::realpath()` tried to access
`$this`, which was undefined, causing a crash.)

`FileSystem::realpath()` looks like a thin wrapper which adds some
special-case handling for D8 stream wrappers.  I don't know anything about
that edge-case, but it seems that using vanilla `realpath()` works better
than misusing `FileSystem::realpath()`.
In my system with the current D8, activating Civi leads to a WSOD with the
message "The website encountered an unexpected error.  Please try again
later.The website encountered an unexpected error.  Please try again later."

There appears to be an issue with handling the `{one}/{two}/{three}/{four}`
trick for capturing subpaths. This uses a different trick for mapping
all subparts of the path to `$extra`.
@totten totten changed the title CRM-17917 - civicrm.install - Fix realpath() call during system setup CRM-17917 - Multiple fixes for fresh Drupal 8 installation Jun 17, 2016
@totten
Copy link
Member Author

totten commented Jun 17, 2016

To setup a new D8 build, I'd suggest at time of writing:

civibuild create d8-master --patch https://github.com/civicrm/civicrm-drupal/pull/388 --patch https://github.com/civicrm/civicrm-core/pull/8592

@seamuslee001
Copy link
Contributor

seamuslee001 commented Oct 12, 2016

Just noting to set up a new D8 Build now you just need

civibuild create d8-master --patch https://github.com/civicrm/civicrm-drupal/pull/388

@@ -29,17 +29,9 @@ public static function create(ContainerInterface $container) {
);
}

public function main($args, $one, $two, $three, $four, $five) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@totten After i reverted this and applied a small patch to the crmNavigationMenu smarty function i got the civicrm menu reliably loading

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seamuslee001 , clarification: was that reverting a specific line or all of 495e085?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also: is the D8 build 8.0, 8.1, or something else? (See core/CHANGELOG.txt.) IIRC, I needed the patch in 8.1 to get any routes working...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@totten yes and i built this off 8.2 using the command above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clarification it was reverting your whole commit 495e085.

@eileenmcnaughton
Copy link
Contributor

@totten any reason not to merge this? My position on 8.x at the moment is that we should merge a bit more agressively if it is to help get it going

@seamuslee001
Copy link
Contributor

@eileenmcnaughton The only issue is that i built off 8.2 using buildkit and had to revert one of Tim's commits to make things work right.

@eileenmcnaughton
Copy link
Contributor

one of the commits in this PR? Not a good advertisement for it then...

@jackrabbithanna
Copy link
Contributor

@seamuslee001 Can you tell me what "crmNavigationMenu smarty function " you had to patch, and how to get this to work?

@jackrabbithanna
Copy link
Contributor

jackrabbithanna commented Jan 5, 2017

@totten , @seamuslee001 , @eileenmcnaughton
Tested these changes...
I found that totten's patches to src/Routing/Route.php and src/Controller/CivicrmController.php
are necessary, and work.
the changes in civicrm.install could be better, I like this PR here for that:
#391 are what should be

Successfully installed CiviCRM into Drupal 8 without civibuild with the changes in the PR and the one mentioned previously.

Drupal 8.2.5
Libraries 8.x-3.x-dev
Jan 4 daily tarball of 4.7

Fixing this bit is good, and should be committed....
I have no CiviCRM menu up top on Civi pages, but the paths themselves work.....that should be a separate issue/PR

@seamuslee001
Copy link
Contributor

Thanks Mark, THe test that i needed was that the navigation menu url (that gets loaded by ajax) wasn't quite working as there was an // in instead of a language so i got a patch into core already which sets it to be en_US if not set otherwise. If you have tested and it works for you I'm happy for this to go through

@jackrabbithanna
Copy link
Contributor

Ok I needed the changes in this PR to just get any CiviCRM page to load....
I still don't see a menu....
In the watchdog log , I'm seeing page not found errors having to do with the ajax path for the menu.....
So maybe what we need here is a tweak to totten's commit....

the page not found is for this path: (I've got the site in localhost, in a subdirectory of htdocs, no vhost entry, htdocs/drupal825)
http://localhost/drupal825/civicrm/ajax/menujs/202/en_US/1/NFmjWSIO

What should the path to the ajax menu callback be?

@totten
Copy link
Member Author

totten commented Jan 6, 2017

Thanks for the feedback @jackrabbithanna.

On my local D7, the path looks very similar -- in HTML, it's /civicrm/ajax/menujs/203/en_US/1/cIVjCDYN (which resolves to http://dmaster.l/civicrm/ajax/menujs/203/en_US/1/cIVjCDYN).

The route civicrm/ajax/menujs has an uncommon property where it actually does take advantage of multiple slashes of subordinate paths. (You can see the route is actually defined as civicrm/ajax/menujs.)

This suggests that the option 'extra' => '.*', isn't having the intended effect (matching the route to all subpaths)?

See also: http://symfony.com/doc/current/routing/slash_in_parameter.html

@colemanw colemanw merged commit 5485096 into civicrm:8.x-master Jan 6, 2017
@colemanw
Copy link
Member

colemanw commented Jan 6, 2017

Merging as most comments suggest this is an improvement on the whole.

@jackrabbithanna
Copy link
Contributor

I'll keep pushing on this in a new PR to get the menu going

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.

6 participants