dev/core#2823 Restructure determination of required actions. #21401
Conversation
|
(Standard links)
|
| } | ||
| public function reconcileEnabledModule(string $module): void { | ||
| foreach ($this->getManagedEntitiesToUpdate(['module' => $module]) as $todo) { | ||
| $dao = new CRM_Core_DAO_Managed(); |
There was a problem hiding this comment.
This $dao building is fugly but I wanted to leave the signatures of the action functions unchanged for this commit & follow up in a later change. Given that I was happy with what looks like a bit of duplication for now
06d1e57 to
abb4304
Compare
|
CRM_Core_ManagedEntitiesTest.testModifyDeclaration_UpdateNever |
fff2cbc to
9d8ffde
Compare
|
@eileenmcnaughton it looks to me like the big functional change in this PR which may be affecting tests is that loading declarations is no longer done "on demand" but now it's done as part of the class constructor, which may mean it's running earlier than it was before, possibly before all extensions have been loaded into the container? I was able to reproduce the test failures locally, running tests inside PhpStorm. |
|
Yeah the 2 commits before that one are passing just fine #21400 There isn't a gap between when the constructor is called and the reconcile from the calling functions point of view... I feel like maybe I should fix the main tests on the ManagedEntities class to use hooks rather than pass in 'decls' so it's working the same way as 'real' code. Maybe if the first 2 are merged I should make that change before trying to change the constructor
|
5f2bbf1 to
7e1f8a2
Compare
|
OK - I think I understand what is happening here - when the ManagedEntities class is constructed using the singleton it uses a copy of it cached in the static. Almost all the places that call this singleton force a flush - but the afform places that are resulting in test fails are a notable exception. I've tried fixing it to use |
|
I should note - we don't have to load these things in the constructor - we could load them later - which is what is happening. But what are we actually caching at that point? Probably only a copy of To wit - why would you want to interact with ManagedEntities with anything other than a fresh copy of what is to be managed? |
|
Hmm - ok - a recursive in there to figure out |
|
Retest this please |
…ic' use The function currently being called will wind up protected civicrm/civicrm-core#21401
7e1f8a2 to
2418497
Compare
|
@colemanw putting the declaration calculation in the constructor seems to be a battle! I've switched tack for now and have ensuring they are calculated in the reconcile function and to offset that have made most of the other functions protected. The reason I didn't do that before is that one of them is called from outside core - but I put up a PR to address that - veda-consulting-company/uk.co.vedaconsulting.gdpr#294 Only these functions are now public - the 'get' one is used in tests & also pretty hard to grep for so not digging too much into it for now but I made it deprecated as it's silly
|
The expected actions are now calculated at the start of reconcile. and retrievable by function. The code that calls the action-actions is still pretty cludgey - but the intent is that we would stop passing the dao object into those action-action functions in the near future. This PR punts any decisions about how outputs might look (eg. making the array retrievable from outside of the function. Note that there is pretty comprehensive test cover in CRM_Core_ManagedEntitiesTest
The only affected universe place I found was veda-consulting-company/uk.co.vedaconsulting.gdpr#294 Since putting the 'information gathering & storing' function on the constructor is tricky due to the singleton caching this means we are always working with a consistent entry point
I'm getting a loop
2418497 to
f9ee37c
Compare
|
@colemanw this is passing now AND there is a regression in master without it now.... |
|
Then let's merge it! |
Overview
dev/core#2823 Restructure determination of required actions.
This builds on top of the 2 cleanups in the other open PRs #21399 & #21440 to build an array of actions for the reconcile enable in the constructor & adds functions to retreive those. It is a part-way refactor
Before
The determination of actions to take & the doing of the actions is interwoven
After
The determination happens in the constructor. The doing happens in 'reconcile'
Technical Details
The code that calls the action-actions
is still pretty cludgey - but the intent is that
we would stop passing the dao object into those
action-action functions in the near future.
Comments