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

Check for no action task so we don't add an undefined page #20945

Closed
wants to merge 1 commit into from

Conversation

mattwire
Copy link
Contributor

Overview

Partial from #20940 which contains the actual fix for the crash (there's a lot of PHP notice fixes there too but the actual fix is a small change so extracted here for ease of review).

This was found following release 1.2 of exportpermissions extension which supports disabling export/print/pdf/labels tasks. It turns out that disabling all of these can cause the advanced search to fail because some "modules" do not have any search tasks - see progressivetech/net.ourpowerbase.exportpermission#6.

Specifically Activities and Mailings trigger a "Network error" when you click the accordion to expand the search criteria because when there are no tasks defined a "null" page gets added which then triggers a require_once('.php') here https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/Controller.php#L452 because $className is not defined.

Before

When a search task has no actions (eg. Activity, Mailings) and it is expanded in Advanced Search it crashes.

After

All tasks can be disabled and everything continues working!

Technical Details

Comments

@civibot
Copy link

civibot bot commented Jul 24, 2021

(Standard links)

@civibot civibot bot added the master label Jul 24, 2021
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Jul 25, 2021
Instead of 'handling the task code building invalid tasks - civicrm#20945' - this proposes to only attempt
to add a page if it is for a valid task. This keeps the grouchiness in the generic controller
to warn us of other mistakes.

This code is pretty fugly! However, it really is just called internally within this class.
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Jul 25, 2021
Instead of 'handling the task code building invalid tasks - civicrm#20945' - this proposes to only attempt
to add a page if it is for a valid task. This keeps the grouchiness in the generic controller
to warn us of other mistakes.

This code is pretty fugly! However, it really is just called internally within this class.
@eileenmcnaughton
Copy link
Contributor

@mattwire so looking at this the 'original sin' is that (for example) Activity_SearchMachine_Search calls

$this->addSequentialPages($this->_pages, $action);

with invalid data in $this->_pages

That bad data seems to come from one of the places you identified as serving up enotices = activityTask::getTask

It doesn't actually seem like like that getTask function is called from anywhere else so I think we can cleanup the enotice in a way that stops it adding invalid pages - #20951

eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Jul 25, 2021
Instead of 'handling the task code building invalid tasks - civicrm#20945' - this proposes to only attempt
to add a page if it is for a valid task. This keeps the grouchiness in the generic controller
to warn us of other mistakes.

This code is pretty fugly! However, it really is just called internally within this class.
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Jul 25, 2021
Instead of 'handling the task code building invalid tasks - civicrm#20945' - this proposes to only attempt
to add a page if it is for a valid task. This keeps the grouchiness in the generic controller
to warn us of other mistakes.

This code is pretty fugly! However, it really is just called internally within this class.
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Jul 25, 2021
Instead of 'handling the task code building invalid tasks - civicrm#20945' - this proposes to only attempt
to add a page if it is for a valid task. This keeps the grouchiness in the generic controller
to warn us of other mistakes.

This code is pretty fugly! However, it really is just called internally within this class.
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Jul 25, 2021
Instead of 'handling the task code building invalid tasks - civicrm#20945' - this proposes to only attempt
to add a page if it is for a valid task. This keeps the grouchiness in the generic controller
to warn us of other mistakes.

This code is pretty fugly! However, it really is just called internally within this class.
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Jul 26, 2021
Instead of 'handling the task code building invalid tasks - civicrm#20945' - this proposes to only attempt
to add a page if it is for a valid task. This keeps the grouchiness in the generic controller
to warn us of other mistakes.

This code is pretty fugly! However, it really is just called internally within this class.
@mattwire
Copy link
Contributor Author

mattwire commented Aug 1, 2021

Closing in favour of #20951 #20989

@mattwire mattwire closed this Aug 1, 2021
@mattwire mattwire deleted the tasksnullpage branch August 1, 2021 18:27
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.

2 participants