Skip to content

Replace Deprecated Class references with their main class name - #68

Open
seamuslee001 wants to merge 1 commit into
twomice:masterfrom
seamuslee001:deprecated_class
Open

Replace Deprecated Class references with their main class name#68
seamuslee001 wants to merge 1 commit into
twomice:masterfrom
seamuslee001:deprecated_class

Conversation

@seamuslee001

Copy link
Copy Markdown
Contributor

As per the code docs here https://github.com/civicrm/civicrm-core/blob/master/api/Exception.php#L16 this replaces deprecated class references with their main class name

cc @colemanw @twomice

@colemanw

Copy link
Copy Markdown
Contributor

@seamuslee001 what inspired this PR? I'm actually auto-opening these on every extension throughout universe at the moment. As it happens the script just got to this extension & opened #69 but I'll close that one since you got to it first.

@seamuslee001

Copy link
Copy Markdown
Contributor Author

@colemanw partially seeing you doing things and also partially because I have been working on improving the code on a client site where this one is installed and figured better to do this upstream

@colemanw

Copy link
Copy Markdown
Contributor

Cool. FYI there were thousands of references to these deprecated classes throughout universe and my script just opened up hundreds of PRs and MRs to fix all of them 🤓

@twomice

twomice commented Oct 23, 2025

Copy link
Copy Markdown
Owner

... the script just got to this extension & opened #69 but I'll close that one since you got to it first.

Does this mean I might have duplicate PRs on dozens of my projects? I'm sure that's better than none at all, but it would make some degree of double work for developers to process the PRs. @seamuslee001 @colemanw

@colemanw

Copy link
Copy Markdown
Contributor

Does this mean I might have duplicate PRs on dozens of my projects?

AFAIK @seamuslee001 only opened this one PR. I'm the one with the machine gun 🔫🤓

@twomice

twomice commented Oct 23, 2025

Copy link
Copy Markdown
Owner

Great. We should totally hang out, then. 👍

@twomice

twomice commented Oct 23, 2025

Copy link
Copy Markdown
Owner

Holding on this PR (and any similar ones in other projects), pending an answer to https://civicrm.stackexchange.com/questions/49930/does-crm-core-exception-support-detecting-api-specific-errors-api-exception-is

@colemanw

Copy link
Copy Markdown
Contributor

Did my best to answer @twomice although I'm afraid the answer's not great.

@twomice

twomice commented Oct 23, 2025

Copy link
Copy Markdown
Owner

+1 for the effort. ❤️
-1 for the answer. 💩

@colemanw

Copy link
Copy Markdown
Contributor

I'll take it. 🤣

I mean, aside from causing you an existential crisis, it doesn't actually block this PR. Like it or not, the classes were merged upstream (don't blame me, @totten did it 😅) so referencing the alias instead of the canonical class name doesn't help you any.

@twomice

twomice commented Oct 23, 2025

Copy link
Copy Markdown
Owner

... the classes were merged upstream ... so referencing the alias instead of the canonical class name doesn't help you any.

Wait, do you mean that catch (CiviCRM_API3_Exception $e) will just never catch, because $e is alread of type CRM_Core_Exception?

@colemanw

Copy link
Copy Markdown
Contributor

@twomice No, I mean that the classes are completely interchangeable. As far as php is concerned it doesn't matter what you call them, they are the same class, so

CRM_Core_Exception === API_Exception === CiviCRM_API3_Exception

Which means you can do something like:

try {
  throw new CRM_Core_Exception('Who cares?');
}
catch (API_Exception $e) {
  print $e->getMessage();
}

This will print "Who cares?". Wild, I know.

@twomice

twomice commented Oct 23, 2025

Copy link
Copy Markdown
Owner

Got it, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants