[REF] Use OO when determining what to suggest for settings.php prefixes for drupal/backdrop views, instead of scattered "if cms ==" #21042

eileenmcnaughton · 2021-08-06T07:52:13Z

Overview

I took a look at #20682 and it seemed clear that
in it's current state it would fix one CMS (d9) & break another (d7). This is
an alternate that will hopefully be non-breaky

Before

See #20682

After

fixed - hopefully without breaking anything else

Technical Details

The discussion by @demeritcowboy @homotechsual @demeritcowboy @seamuslee001 seems to indicate this would be a be a safer approach

Comments

ping @pradpnayak

civibot · 2021-08-06T07:52:15Z

(Standard links)

If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

demeritcowboy · 2021-08-06T13:34:56Z

See also eileenmcnaughton/civicrm_entity#312 (comment) which was just merged recently. I'm not sure at the moment whether that makes this unnecessary or is separate. @jackrabbithanna @KarinG

eileenmcnaughton · 2021-08-06T15:02:41Z

Oh yeah - I saw @KarinG pasted that link on the original but I didn't actually click through on it - I see what you mean

herbdool · 2021-08-07T00:12:20Z

I haven't looked too closely but I recall us having an issue with civi integration with views in Backdrop but it was because of a new support for latest MySQL. @laryn do you know if there's an issue someplace?

jackrabbithanna · 2021-08-07T00:29:55Z

Recently Matt made a PR where it is no longer necessary to add the "views integration" database prefix settings in the Drupal's settings.local.php

Also, the PR you referenced did fix a bug that caused an SQL that is that error or a similar error as posted for this issue.

Perhaps test Civi without your PR with the latest of the 8.x-3.x branch in the github repo, for Drupal 8/9 I'm not sure any fixes are necessary in Civi Core, but I could be wrong.

eileenmcnaughton · 2021-08-07T00:41:06Z

I'm gonna close this out if people don't think it's needed

herbdool · 2021-08-07T03:50:27Z

As far as Backdrop is concerned this PR is still an improvement by moving the logic to the userframework class. It might not address the official bug but it's still good.

I found: backdrop/backdrop-issues#4745. I believe that we'll still need the separate logic between Drupal 7 and Backdrop.

Can we revive this PR @eileenmcnaughton or does it need a new issue?

eileenmcnaughton · 2021-08-07T04:24:53Z

@herbdool - tada - it's back - actually you are right - I didn't include any actual change as far as I can tell - so no reason not to merge this

demeritcowboy · 2021-08-07T04:38:21Z

CRM/Utils/System/Base.php

+   */
+  public function getCMSDatabasePrefix(): string {
+    $crmDatabase = DB::parseDSN(CRM_Core_Config::singleton()->dsn)['database'];
+    $cmsDatabase = DB::parseDSN(CRM_Core_Config::singleton()->dsn)['database'];


??? This looks wrong.

does doesn't it....

and yet, like magic....

Looks better now

herbdool · 2021-08-07T11:20:11Z

I haven't tested but looks like it'll work the same as before. LGTM

KarinG · 2021-08-07T20:06:05Z

What is the issue that is being fixed here?

Drupal 9 / Views with latest CiviCRM entity PR 312 is working with Drupal and CiviCRM tables in a single database as well as with Drupal and CiviCRM tables in two separate databases ->without<- (as this has been deprecated) specifying

$databases['default']['default']['prefix']= array(
'default' => '',
'civicrm_acl'

in the settings.php file

And it’s working for very complicated Views involving Views relationships joining both Drupal and CiviCRM tables.

And we have added a first Views automated test in CiviCRM Entity now.

I thought all things Views should live in CiviCRM Entity - that’s why I had Matt help me make this possible via CiviCRM Entity.

@pradpnayak - who first posted referenced an SE issue w/ database prefixes - but those are deprecated - so what’s the issue that needs fixing post CiviCRM Entity PR 312?

herbdool · 2021-08-08T03:13:53Z

@KarinG I mentioned above why I asked to reopen this PR.

This is not so much about d9 as about just putting existing logic in the right place. The prefix setting is better in the class.

Maybe the PR could be renamed?

demeritcowboy · 2021-08-08T03:41:15Z

Some thoughts:

What this form does is display some stuff on screen, it's not directly involved in how views integrates. Ideally it would display the correct thing to avoid frustration when setting up a new site (for drupal 7/backdrop), but no existing sites will suddenly break from this.
I'd suggest not adding the drupal 9 class because it doesn't do anything because it never gets called since there is no CIVICRM_UF=Drupal9 and likely never will be.
It previously compared the full DSN. Now it only compares the db name. That should be ok though.
When the next civicrm_entity is released a followup PR could be done to display something to the user for Drupal8+ that it's no longer needed (if I'm understanding correctly).
It would be good to at least give this a minimal run. Has anyone run it yet?

demeritcowboy · 2021-08-08T21:42:48Z

CRM/Utils/System/Base.php

+    if ($crmDatabase === $cmsDatabase) {
+      return '';
+    }
+    return "`$cmsDatabase`.";


I was just testing this (drupal 7) - this is backwards - you need the crmdatabase.

@demeritcowboy do you want to push in the changes that this PR needs?

demeritcowboy · 2021-08-08T21:45:16Z

There was a larger question raised:

I thought all things Views should live in CiviCRM Entity

Would it make sense to just take this section of the form completely out of core and let civicrm_entity provide a page to tell the user whatever it thinks is right?

herbdool · 2021-08-09T14:55:31Z

@demeritcowboy wouldn't your proposal mean that all sites regardless of CMS would need to install that module? I don't think that would work. Not all D7 sites would need the full civicrm_entity capability and the module doesn't currently exist for Backdrop.

KarinG · 2021-08-09T15:18:09Z

As of D8 / D9 all views integration is supported by CiviCRM entity

D7 views integration should stay as is - CiviCRM entity is not and should not be required.

Still not at all clear to me what ‘Drupal 9 views fix’ is being fixed here. We have a lot of Drupal 9 views that are working fine especially after we sorted out civicrm entity to handle single vs separate database scenarios. If the title of this PR is wrong can someone please suggest a better one with steps to reproduce?

demeritcowboy · 2021-08-09T16:02:25Z

@KarinG I've changed the title.

@herbdool Good to know civicrm_entity doesn't work with backdrop. And yes right even drupal 7 doesn't require it for views.

To recap then:

Backdrop: No backticks.
Drupal 7: Backticks
Drupal 8: The prefixes aren't needed if you are running off the latest civicrm_entity from git, but still needed if you are running off the latest download of the module from drupal.org. Not sure what composer does (nobody knows what composer does).
Drupal 9: Might need something different from drupal 8, but people are encouraged to use the latest civicrm_entity from git instead.
Drupal 10: It will NOT work if using an older civicrm_entity regardless of prefixes or not - you MUST use the latest civicrm_entity from git.

Side note: Line 76 references a non-existent variable.

KarinG · 2021-08-09T16:14:50Z

Thanks Dave 👍

@jackrabbithanna -> can you please queue up a release for CiviCRM Entity? Then >= D8 views will -no longer- require prefixes regardless of whether people use a single or separate db-s - until then people are going to hit ‘something not working’ and will look in CiviCRM core to fix it.

seamuslee001 · 2021-08-09T23:52:50Z

So my other question is that does D7 still work with Backticks or not, given that Backdrop effectively ported the recent D7 patches for MySQL 8 which is what caused the backticks to no longer work on Backdrop, does D7 still need them.

As for composer if you just use drupal/civicrm_entity then it depends on what is in the drupal releases / git repo in drupal's world but if you specify to use the github then it will be what ever is in the Eileen repo setup.

eileenmcnaughton · 2021-08-10T00:02:28Z

@demeritcowboy my reason for adding an empty d9 class is that say in future we Do have a function that differs between 8 & 9. Then we could have upgrading fun - ie you can't set d9 on your pre-upgraded DB because it isn't there until the code base supports it but you really want to set it before you put the new code base there (I'm remembering this being tricky with d6->d7) - of course in writing this I realised we'd need to add a permissions stub too

jackrabbithanna · 2021-08-10T00:07:27Z

@KarinG still having some consequences of the activity and event entity types having bundles based on the event type and activity type .. that layout builder issue I mentioned .. but maybe this Views stuff is more important and worth a release.

eileenmcnaughton · 2021-08-10T00:08:15Z

@jackrabbithanna - if it doesn't break anything then release it :-)

jackrabbithanna · 2021-08-10T00:13:22Z

Its a different feature, but got to choose the lesser of two evils sometimes

eileenmcnaughton · 2021-08-25T22:45:52Z

@demeritcowboy I think this is mergeable but there has been a lot of tangential conversation - is that your take?

demeritcowboy · 2021-08-25T22:50:31Z

Hi,
It's not mergeable. For one it has the crm and cms backwards.
It sounded like there was a plan to make a release for civicrm_entity with the latest goodies, but I don't see that on drupal.org yet.
I'd maybe suggest closing this for now.

herbdool · 2021-08-25T22:58:49Z

civicrm_entity is a separate issue at this point. Doesn't affect the merits of this. Other than fixing that one error I think it's mergable

demeritcowboy · 2021-08-25T23:28:16Z

civicrm_entity is relevant in the sense that if we can drop any code related to drupal 8+ then that has more merit, and as noted in the comments possibly even the drupal 7 part can go.

This PR takes non-trivial time to test - obviously nobody tested it - I'd rather not spend more of my time on this version if it's going to be changing anyway - I stopped when the crm/cms thing came up, so there's still more to test.

eileenmcnaughton · 2021-08-25T23:31:38Z

@demeritcowboy ok - I'll close

herbdool · 2021-08-25T23:38:45Z

@demeritcowboy How can the D7 part be removed? Is civicrm_entity going to be required for all Drupal sites?

Regarding the empty D9 class I thought that was already explained above. It's not just related to Views.

I haven't had a chance yet to test it but I figured it was good on the principle of better refactoring.

demeritcowboy · 2021-08-25T23:40:15Z

D7 removed in the sense it might now be the same as backdrop.

eileenmcnaughton · 2021-08-25T23:49:35Z

I just took a quick look & fixed the crm vs cms & re-opened based on @herbdool - but this does require testing on at minimum backdrop & at least one other cms I guess

I don't have strong opinions about push through vs abandon but I do think this is cleaner & will make it easier to add d9 overrides if & when the time comes

herbdool · 2021-08-26T00:25:53Z

I'll try to test it tomorrow. Or after my time off in a couple weeks.

seamuslee001 · 2021-08-26T02:50:36Z

CRM/Utils/System/Base.php

+   * However, this string should contain backticks, or not, in accordance with the
+   * CMS's drupal views expectations, if any.
+   */
+  public function getCRMDatabasePrefix(): string {


I think that this should just return '' and this code should actually sit in the DrupalBase.php class which is shared by the Drupal 7 (Drupal.php), Backdrop and Drupal 8 classes as appropriately

@seamuslee001 do you mean the default is returning an empty string and then in the Drupal 7 class and Backdrop class add a check if $crmDatabase === $cmsDatabase?

@seamuslee001 the reason I landed on the base class is that it is still a valid db prefix beyond drupal - even if it isn't used

herbdool · 2021-08-26T20:20:18Z

CRM/Admin/Form/Setting/UF.php

-      $dsn = CRM_Utils_SQL::autoSwitchDSN($config->dsn);
-      $dsnArray = DB::parseDSN($dsn);
+
+      $dsnArray = DB::parseDSN(CRM_Utils_SQL::autoSwitchDSN($config->dsn));
      $tableNames = CRM_Core_DAO::getTableNames();
      asort($tableNames);
      $tablePrefixes = '$databases[\'default\'][\'default\'][\'prefix\']= [';


Unfortunately we still have a $config->userFramework === 'Backdrop' just below this. Do we need another method for $tablePrefixes? Though I see other references above as well to WordPress, Drupal 8, etc so maybe the whole buildForm should be passed in from the classes? Or leave that for a future issue.

Let's leave out of scope of this given how stalled it is

herbdool · 2021-08-26T20:22:40Z

I've tested in Backdrop and the PR looks fine. I won't have time to test Drupal7 for a couple weeks.

herbdool · 2021-08-26T20:24:00Z

@demeritcowboy I'm not sure if Drupal 7 is now the same as Backdrop. I recall that they took a slightly different approach and not sure if Drupal 7 took away the backticks. I'm assuming it's backwards compatible?

eileenmcnaughton · 2021-09-02T21:58:20Z

@demeritcowboy @seamuslee001 where do you think this is at given @herbdool's testing (above)

I just re-read through the code & I believe that for d7 (untested) & d8 (tested) the db would have backticks before and after this change

herbdool · 2021-09-13T16:22:49Z

@eileenmcnaughton I've now tested in d7 as well. Before and after, the format looks the same. Like

'`civicrm`.'

So I believe this meets the goal of refactoring here. Code is better, output is the same.

eileenmcnaughton · 2021-09-13T21:34:04Z

@demeritcowboy @seamuslee001 ^^

seamuslee001 · 2021-09-13T21:40:14Z

CRM/Utils/System/Drupal9.php

+/**
+ * Drupal specific stuff goes here.
+ */
+class CRM_Utils_System_Drupal9 extends CRM_Utils_System_Drupal8 {}


@eileenmcnaughton I genuinely don't see a need for this at all given that we are not changing the UF between Drupal 8 -> Drupal 9

@seamuslee001 yeah - it feels 'right' to me to put this here because it seems inevitable there will be some variance at some point - but TBH my involvement in this one is just that I was trying to resolve an old stalled PR. (I've already tried to close it twice :-))

I would merge it if this was not included

OK - I've removed it - I guess putting this to bed is more important than my feeling it should be added

seamuslee001 · 2021-09-13T21:40:50Z

I would also just add that I think I tested this on d7 but without the backticks and that seem to work as well (maybe some compatibility layer exists there somewhere)

I took a look at civicrm#20682 and it seemed clear that in it's current state it would fix one CMS (d9) & break another (d7). This is an alternate that will hopefully be non-breaky

civibot bot added the master label Aug 6, 2021

eileenmcnaughton mentioned this pull request Aug 6, 2021

Syntax error or access violation on D9 views #20682

Closed

eileenmcnaughton closed this Aug 7, 2021

eileenmcnaughton reopened this Aug 7, 2021

demeritcowboy reviewed Aug 7, 2021

View reviewed changes

eileenmcnaughton force-pushed the uf branch from ffb2ea5 to 7fa5101 Compare August 7, 2021 04:46

demeritcowboy reviewed Aug 8, 2021

View reviewed changes

demeritcowboy changed the title ~~drupal 9 views fix (alternative)~~ [REF] Use OO when determining what to suggest for settings.php prefixes for drupal/backdrop views, instead of scattered "if cms ==" Aug 9, 2021

eileenmcnaughton closed this Aug 25, 2021

eileenmcnaughton reopened this Aug 25, 2021

eileenmcnaughton force-pushed the uf branch from 7fa5101 to e601d46 Compare August 25, 2021 23:45

seamuslee001 reviewed Aug 26, 2021

View reviewed changes

herbdool reviewed Aug 26, 2021

View reviewed changes

seamuslee001 reviewed Sep 13, 2021

View reviewed changes

Views db cleanup (alternative)

6440c50

I took a look at civicrm#20682 and it seemed clear that in it's current state it would fix one CMS (d9) & break another (d7). This is an alternate that will hopefully be non-breaky

eileenmcnaughton force-pushed the uf branch from e601d46 to 6440c50 Compare September 13, 2021 22:20

seamuslee001 added the merge on pass label Sep 13, 2021

eileenmcnaughton merged commit a20a1f6 into civicrm:master Sep 14, 2021

eileenmcnaughton deleted the uf branch September 14, 2021 01:39

[REF] Use OO when determining what to suggest for settings.php prefixes for drupal/backdrop views, instead of scattered "if cms ==" #21042

[REF] Use OO when determining what to suggest for settings.php prefixes for drupal/backdrop views, instead of scattered "if cms ==" #21042

Conversation

eileenmcnaughton commented Aug 6, 2021 • edited Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented Aug 6, 2021

demeritcowboy commented Aug 6, 2021

eileenmcnaughton commented Aug 6, 2021

herbdool commented Aug 7, 2021

jackrabbithanna commented Aug 7, 2021

eileenmcnaughton commented Aug 7, 2021

herbdool commented Aug 7, 2021 • edited Loading

eileenmcnaughton commented Aug 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

herbdool commented Aug 7, 2021

KarinG commented Aug 7, 2021 • edited Loading

herbdool commented Aug 8, 2021 • edited Loading

demeritcowboy commented Aug 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demeritcowboy commented Aug 8, 2021

herbdool commented Aug 9, 2021

KarinG commented Aug 9, 2021

demeritcowboy commented Aug 9, 2021

KarinG commented Aug 9, 2021

seamuslee001 commented Aug 9, 2021

eileenmcnaughton commented Aug 10, 2021

jackrabbithanna commented Aug 10, 2021

eileenmcnaughton commented Aug 10, 2021

jackrabbithanna commented Aug 10, 2021

eileenmcnaughton commented Aug 25, 2021

demeritcowboy commented Aug 25, 2021

herbdool commented Aug 25, 2021 • edited Loading

demeritcowboy commented Aug 25, 2021

eileenmcnaughton commented Aug 25, 2021

herbdool commented Aug 25, 2021

demeritcowboy commented Aug 25, 2021

eileenmcnaughton commented Aug 25, 2021

herbdool commented Aug 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

herbdool commented Aug 26, 2021

herbdool commented Aug 26, 2021

eileenmcnaughton commented Sep 2, 2021

herbdool commented Sep 13, 2021 • edited Loading

eileenmcnaughton commented Sep 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seamuslee001 commented Sep 13, 2021

eileenmcnaughton commented Aug 6, 2021 •

edited

Loading

herbdool commented Aug 7, 2021 •

edited

Loading

KarinG commented Aug 7, 2021 •

edited

Loading

herbdool commented Aug 8, 2021 •

edited

Loading

demeritcowboy commented Aug 8, 2021 •

edited

Loading

herbdool commented Aug 25, 2021 •

edited

Loading

herbdool commented Sep 13, 2021 •

edited

Loading