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

Move BAO and template files into event cart #17743

Merged
merged 1 commit into from
Jul 14, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jul 3, 2020

Overview

This is a partial of #17339 - it moves the BAO and template files to the extension and does a limited install - but leaves the more complex work to be done in later PRs.

Note that the install in this PR only works because no tables are created as part of the extension. There are technical issues still to be solved before we can have a core-extension that does anything on install. As a hidden extension this must remain enabled and we use the hidden tag to denote that this is part of core, not an add-on extension. There may come a time when it can be disabled but today is not that day.

Before

BAO files in original place

After

BAO files moved

Technical Details

#/17339 also incorporates work to move to a more extension-like interaction with core (using hooks / api end points ) and towards solving the install issue - this PR just aims to do the technically simple but vey stale-prone part.

Comments

@civibot
Copy link

civibot bot commented Jul 3, 2020

(Standard links)

@eileenmcnaughton
Copy link
Contributor Author

Added OK without test as this is the file moving part - adding tests can't really happen until the code is in the right place - it's a bit chicken & egg but this removes blockers to adding tests

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 you were going to check if all the enable steps were OK here

@seamuslee001
Copy link
Contributor

yes this was on my things to do

@mattwire
Copy link
Contributor

@seamuslee001 @eileenmcnaughton This is failing on:

CRM_Member_Form_MembershipRenewalTest::testSubmitRecurCompleteInstant
Failed asserting that '2' matches expected 1.

That must be unrelated?

@mattwire
Copy link
Contributor

We also need the changes from bin/regen.sh and .gitignore from #17339 here

@eileenmcnaughton
Copy link
Contributor Author

@mattwire I double checked & the .gitignore bit is already done. I added the regen.sh here #17839 since it's a bit staley, applies to other extensions, and is crying out for a better fix.

Test fail patch in #17830

@seamuslee001
Copy link
Contributor

I tested this and found only one issue which was the templates folder path wasn't matching the standard folder path (was missing the Cart subdirectory) after fixing that I can confirm the event cart system works as expected also tested and confirmed the upgrade works as expected as well.

@seamuslee001
Copy link
Contributor

Test fail unrelated

@seamuslee001 seamuslee001 merged commit b8b24ec into civicrm:master Jul 14, 2020
@seamuslee001 seamuslee001 deleted the eventcart branch July 14, 2020 23:56
totten added a commit to totten/civicrm-core that referenced this pull request Jul 16, 2020
Overview
--------

This fixes a recent issue in which `setup.sh` is producing local-only changes to the `Cart`
and `EventInCart` DAO files.

To reduce ambiguity, I'm rephrasing the normal "Before/After" and giving a 3-step evolution/history.

Evolution
---------

(1) Traditionally, the `Cart`/`EventInCart` entities have both BAO+DAO in main source-tree.

(2) With civicrm#17743 (685bf3d), the BAO moved away. But then
we get some detritus in the code-tree whenever you run `setup.sh`.

(3) With this patch, we don't get the detritus anymore.

Technical Details
-----------------

When I ran `setup.sh -g`, it produced local modifications like this:

```
diff --git a/CRM/Event/Cart/DAO/Cart.php b/CRM/Event/Cart/DAO/Cart.php
index 4683add13d..3a7f721dc1 100644
--- a/CRM/Event/Cart/DAO/Cart.php
+++ b/CRM/Event/Cart/DAO/Cart.php
@@ -94,7 +94,7 @@ class CRM_Event_Cart_DAO_Cart extends CRM_Core_DAO {
           'where' => 'civicrm_event_carts.id',
           'table_name' => 'civicrm_event_carts',
           'entity' => 'Cart',
-          'bao' => 'CRM_Event_Cart_BAO_Cart',
+          'bao' => 'CRM_Event_Cart_DAO_Cart',
           'localizable' => 0,
           'add' => '4.1',
         ],
```

(Let's ignore the cyclic dependency in this design - that's a messier/pre-existing
problem.)

That metadata is inaccurate.  The `bao` should point to the BAO, sinec the BAO does exist.

It's inaccurate because the BAO exists in a different location.

This patch allows you override the file-existence check -- saying, "this entity will
use BAO's even though they're not in the file-location you might normally expect."
christianwach pushed a commit to christianwach/civicrm-core that referenced this pull request Jul 18, 2020
Overview
--------

This fixes a recent issue in which `setup.sh` is producing local-only changes to the `Cart`
and `EventInCart` DAO files.

To reduce ambiguity, I'm rephrasing the normal "Before/After" and giving a 3-step evolution/history.

Evolution
---------

(1) Traditionally, the `Cart`/`EventInCart` entities have both BAO+DAO in main source-tree.

(2) With civicrm#17743 (685bf3d), the BAO moved away. But then
we get some detritus in the code-tree whenever you run `setup.sh`.

(3) With this patch, we don't get the detritus anymore.

Technical Details
-----------------

When I ran `setup.sh -g`, it produced local modifications like this:

```
diff --git a/CRM/Event/Cart/DAO/Cart.php b/CRM/Event/Cart/DAO/Cart.php
index 4683add13d..3a7f721dc1 100644
--- a/CRM/Event/Cart/DAO/Cart.php
+++ b/CRM/Event/Cart/DAO/Cart.php
@@ -94,7 +94,7 @@ class CRM_Event_Cart_DAO_Cart extends CRM_Core_DAO {
           'where' => 'civicrm_event_carts.id',
           'table_name' => 'civicrm_event_carts',
           'entity' => 'Cart',
-          'bao' => 'CRM_Event_Cart_BAO_Cart',
+          'bao' => 'CRM_Event_Cart_DAO_Cart',
           'localizable' => 0,
           'add' => '4.1',
         ],
```

(Let's ignore the cyclic dependency in this design - that's a messier/pre-existing
problem.)

That metadata is inaccurate.  The `bao` should point to the BAO, sinec the BAO does exist.

It's inaccurate because the BAO exists in a different location.

This patch allows you override the file-existence check -- saying, "this entity will
use BAO's even though they're not in the file-location you might normally expect."
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