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

WIP: CRM-20751: Support Drupal aliases for event links in Views #455

Merged

Conversation

twomice
Copy link
Contributor

@twomice twomice commented Jun 20, 2017

@twomice twomice changed the title CRM-20751: Support Drupal aliases for event links in Views WIP: CRM-20751: Support Drupal aliases for event links in Views Jun 20, 2017
@twomice twomice force-pushed the CRM-20751_drupal_alias_views_event branch from 9914e49 to 00406a5 Compare June 20, 2017 19:54
@twomice
Copy link
Contributor Author

twomice commented Jun 20, 2017

Hi @colemanw I'd like to get your feedback on this please. BTW, there are civilint errors that were already there before; is it preferable for me to clean them up in this PR?

@colemanw
Copy link
Member

@twomice I'm curious to know why not patch the civicrm_views_href and civicrm_views_url functions to check drupal path aliases - it's a nice central place to put in this feature.

@colemanw
Copy link
Member

@twomice for that matter, why not have those functions pass the job off to Drupal and skip the civicrm_initialize and CRM_Utils_ stuff.

@twomice
Copy link
Contributor Author

twomice commented Jun 21, 2017

That would expand the scope to "all of CiviCRM Views" instead of just "Events in Views", but yeah, I'm cool with that. Will give it a try.

@colemanw
Copy link
Member

colemanw commented Jun 21, 2017 via email

@twomice
Copy link
Contributor Author

twomice commented Jun 21, 2017

Jenkins test this please.

@twomice twomice force-pushed the CRM-20751_drupal_alias_views_event branch from d3067ce to b7d7ee8 Compare June 21, 2017 21:07
@twomice
Copy link
Contributor Author

twomice commented Jun 21, 2017

@colemanw Right, that makes sense. So here it is. This looks right and works for me.

@colemanw colemanw merged commit f982a2e into civicrm:7.x-master Jun 21, 2017
@colemanw
Copy link
Member

I like it.

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.

3 participants