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

dev/core#1987: Fix theme configuration section on Display preference and improve isFrontendPage function for Drupal CMS. #18322

Closed
wants to merge 2 commits into from

Conversation

swastikpareek
Copy link
Contributor

Overview

Drupal webform pages may have CiviCRM fields which internally initialize CiviCRM assets and was loading CiviCRM current theme assets. This is because CiviCRM doesn't have the logic for checking Drupal webform pages, whether they are FE pages or backend and since the logic is incomplete it loads the backend theme which because of this logic.
Also, CiviCRM, to handle front end pages theme (public pages) and backend pages theme (CiviCRM pages) separately contains a configuration under civicrm/admin/setting/preferences/display?reset=1. Where it provides a configuration field for the backend and the front end theme separately.
This works for all CMS except Drupal. For Drupal, it only had the Theme option which configures only the backend theme. This PR enables both the options for Drupal CMS too and also makes sure the correct theme (front end theme) is loaded on Drupal webform pages which loads CiviCRM components.

Before

3ffa703e-9774-4224-abb0-c967b809dbef

Also, the CiviCRM was loading backend theme for the Drupal public web form pages which loads CiviCRM components.

After

3403e6d0-6a80-4e20-ac50-bd24190cc858

Also, the CiviCRM was loading the front theme for the Drupal public web form pages which loads CiviCRM components.

Technical Details

  • Display.tpl was fixed so that both front end and backend theme configuration was provided to the end-user
  • isFrontEndPage is updated so that it treats Drupal public web form pages as CiviCRM public pages and load front end theme there.

@civicrm-builder
Copy link

Can one of the admins verify this patch?

@civibot
Copy link

civibot bot commented Sep 2, 2020

(Standard links)

@civibot civibot bot added the master label Sep 2, 2020
@mattwire
Copy link
Contributor

mattwire commented Sep 2, 2020

@swastikpareek Thankyou. Can you submit two separate PRs (link to the same lab issue #1987) for the two issues. I agree that both issues should be fixed but they are different things.

@@ -660,7 +660,7 @@ public function isFrontEndPage() {

// Get the menu for above URL.
$item = CRM_Core_Menu::get($path);
return !empty($item['is_public']);
return (empty($item) || !empty($item['is_public']));
Copy link
Contributor

Choose a reason for hiding this comment

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

@swastikpareek Can you add comments here explaining why (use the comments you already made on dev/core#1987)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure will do.

@seamuslee001
Copy link
Contributor

Jenkins add to whitelist

@totten
Copy link
Member

totten commented Sep 3, 2020

I think this is good idea.

The old conditional was put in there before the Drupal integration officially made a distinction (eg before 68e3bc3). Now that it makes the distinction, it's important to show both settings.

@eileenmcnaughton
Copy link
Contributor

I think we don't need a test as it is just removing logic & form layer

@swastikpareek
Copy link
Contributor Author

As per @mattwire, the PR is split into two different PR as they both are different issues

Please let me know if anything else needs to be done here.

Closing this PR. Thanks.

cc @totten @seamuslee001 @eileenmcnaughton

@swastikpareek swastikpareek deleted the issue_1987 branch September 7, 2020 08:17
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