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

Fix fatal error on loading extension page when an extension has been deleted #16752

Merged
merged 1 commit into from
Mar 16, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Mar 11, 2020

Overview

Softens the scenario where an installed extension has been deleted, allowing the extensions page to still load

Before

  1. install an extension
  2. delete the installed extension
  3. load the extension page

Screen Shot 2020-03-12 at 12 22 08 PM

After

Screen Shot 2020-03-12 at 12 35 37 PM

Technical Details

Since we added the concept of 'hidden' tags we are loading them in a way where the exceptions are not caught. This means
that the whole page cannot load - which is needed to disable the missing extension

As the hidden tag / core extension concept was added in 5.24 I'm targetting 5.24

Comments

The messages are possibly a bit overkill but also not necessarily in a bad way

…deleted

Since we added the concept of 'hidden' tags we are loading them in a way where the exceptions are not caught. This means
that the whole page cannot load - which is needed to disable the missing extension
@civibot
Copy link

civibot bot commented Mar 11, 2020

(Standard links)

@civibot civibot bot added the 5.24 label Mar 11, 2020
@eileenmcnaughton
Copy link
Contributor Author

test this please

@seamuslee001
Copy link
Contributor

So in theory lets say you deleted a payment processor extension would you not want that to be a hard fail but maybe that hard fails in other ways as well. Not opposed to this but would appreciate @totten comment on this

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 the problem is it hard fails on the manage extensions page- so you can't disable it.

I am sure a missing payment processor extension would hard fail lots of places TBH

$this->keyToInfo($key);
}
catch (CRM_Extension_Exception_ParseException $e) {
CRM_Core_Session::setStatus(ts('Parse error in extension: %1', [
Copy link
Contributor

Choose a reason for hiding this comment

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

This error sounds a bit developer-y?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah - but it was already in use elsewhere - I just copied it up a level

@mattwire
Copy link
Contributor

Missing payment processors used to kill CiviCRM in lot's of horrible ways. As of about 2 years they haven't done... We shouldn't have any situations where a missing extension prevents access to admin pages related to extensions / status so I'm in favour of this PR, though I haven't fully reviewed or tested it!

@seamuslee001
Copy link
Contributor

I guess my thinking was more along the lines of is the fact that we would be allowing the system work smoother are there any consequences of that practice. I don't think so but just thought worth verbalising that thinking. I agree with matt that the message seems to be bit developery but it is already there so at least we are being consistent. I'm going to merge this as the code looks correct to me and the unit tests cover a number of interactions with exts

@seamuslee001 seamuslee001 merged commit a1ef69d into civicrm:5.24 Mar 16, 2020
@seamuslee001 seamuslee001 deleted the ext24 branch March 16, 2020 20:37
@eileenmcnaughton
Copy link
Contributor Author

thanks @seamuslee001

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