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

Import: Set user context and gracefully handle datasource errors #25171

Closed
wants to merge 1 commit into from

Conversation

mattwire
Copy link
Contributor

Overview

Applies more to SQL datasource which will throw an exception if the query does not work (eg. database table does not exist).

Before

Ugly fatal error screen.

After

Bounces back to datasource form and display error as standard notification.

Technical Details

I can't see anywhere where CONST PATH is used? So I've changed it to a class property to match other forms.

Comments

@eileenmcnaughton

@eileenmcnaughton
Copy link
Contributor

This makes sense but I have some minor stylistic thoughts...

  1. I've been pulling out references to properties with the leading _ (except in those few classes where there is some magic for it) - but in this case it would be better to use a function anyway IMHO because it would be consistent with the getUserJobType() directly below and because it's easier to add logic to when over-riding (rather than refactor later if that comes up)

  2. Can we be more specific with the exception we catch? Potentially instantiateDataSource would catch it & throw our CRM exception (or even better a new CRM_Query_Error exception )

@eileenmcnaughton
Copy link
Contributor

Also - side note - I see us removing const IMPORT_ENTITY = 'Activity'; - I put up #25173 but didn't remove the lines cos it would be hard to juggle with this PR

* Variable to store redirect path.
* @var string
*/
protected $_userContext = 'civicrm/import/activity';
Copy link
Contributor

Choose a reason for hiding this comment

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

@mattwire I took a look at this & all the userContext stuff can be handled by this one-liner

https://github.com/civicrm/civicrm-core/pull/25600/files#diff-f21fc18c9e7abe639a59c87eb679d2983815b234c2282897eeb57086c26a7888R32

If, as I suspect, my alternate PR gets irrevocably bogged down by the handling for the Exception I put in there we can update this PR with that - in the meantime I'll put up a PR to remove PATH and IMPORT_ENTITY per this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

OK - this PR does that #25601

@eileenmcnaughton
Copy link
Contributor

Closing as #25600 replaces

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants