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.

Sign up for GitHub

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

Jump to bottom

[REF] Remove redundant uf templates #31109

Merged
merged 2 commits into from
Nov 16, 2024
Merged

[REF] Remove redundant uf templates #31109

merged 2 commits into from
Nov 16, 2024

Conversation

ufundo
Copy link
Contributor

@ufundo ufundo commented Sep 12, 2024

Before

  • templates for each UF, which all just include the same common template

After

  • just call the common template directly

Technical Details

I suppose in theory you could override the different CMS templates with per-UF variants in a single extension, to achieve different behaviours on different UFs.

But that would be quite a strange thing to do I think. Direct template overrides aren't recommended.

Comments

I'll miss Drupal6...

@civibot civibot
Copy link

civibot bot commented Sep 12, 2024

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers...

➡️ Online demo of this PR 🔗

@civibot civibot bot added the master label Sep 12, 2024
@ufundo ufundo marked this pull request as ready for review September 12, 2024 20:18
seamuslee001
seamuslee001 reviewed Sep 12, 2024
View reviewed changes
CRM/Utils/System/Base.php
$config = CRM_Core_Config::singleton();
return 'CRM/common/' . strtolower($config->userFramework) . '.tpl';
// Despite what the template is called
return 'CRM/common/CMSPrint.tpl';
Copy link
Contributor

Choose a reason for hiding this comment

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

This change would mean that this template file wouldn't be picked up https://github.com/civicrm/civicrm-core/blob/master/templates/CRM/common/joomla.tpl when i think it probably should

Copy link
Contributor Author

@ufundo ufundo Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ahhhh I always forget Joomla!
Thanks @seamuslee001 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm looking at the discrepancies between joomla.tpl and CMSPrint.tpl and wondering if they are mainly because other people have forgotten that separate Joomla template exists when making changes.

I.e. 4e6258a - is there any reason this wouldn't make sense on Joomla?

The other substantive changes I can see is there is an extra crm-content div wrapper inside the crm-container wrapper. Does it do anything?

And the breadcrumbs seem to come through with {$crumb.url} and {$crumb.title} on Joomla, whereas CMSPrint just outputs { $crumb } .

🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have added override for Joomla for now though I feel like it would be good if they could be converged.

demeritcowboy
demeritcowboy reviewed Sep 12, 2024
View reviewed changes
CRM/Utils/System/Base.php Outdated
@@ -78,15 +78,11 @@ abstract public function loadBootStrap($params = [], $loadUser = TRUE, $throwErr
* or equal 0 if not in print mode.
*/
public static function getContentTemplate($print = 0): string {
if ($print === CRM_Core_Smarty::PRINT_JSON) {
return 'CRM/common/snippet.tpl';
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a memory that this was needed for something, like string vs int issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

#30510

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, thanks php...

Also wow thanks @demeritcowboy - you have a great eye/memory!

I'll remove this commit

@ufundo ufundo force-pushed the remove-redundant-uf-templates branch from 84aaa18 to f019cd6 Compare September 13, 2024 10:37
@ufundo ufundo force-pushed the remove-redundant-uf-templates branch from f019cd6 to bb5ac01 Compare September 13, 2024 12:17
@ufundo ufundo changed the title Remove redundant uf templates [REF] Remove redundant uf templates Nov 4, 2024
@demeritcowboy demeritcowboy added the run-extended-tests Civibot should run comprehensive test suites with versions of UF/PHP/MySQL label Nov 16, 2024
@demeritcowboy demeritcowboy merged commit 5eaf0f5 into civicrm:master Nov 16, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seamuslee001 seamuslee001 seamuslee001 left review comments

@demeritcowboy demeritcowboy demeritcowboy left review comments

Assignees
No one assigned
Labels
master run-extended-tests Civibot should run comprehensive test suites with versions of UF/PHP/MySQL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ufundo @demeritcowboy @seamuslee001