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

getStatus() should be returning a string #25441

Merged
merged 1 commit into from
Feb 20, 2023

Conversation

MegaphoneJon
Copy link
Contributor

@MegaphoneJon MegaphoneJon commented Jan 26, 2023

Overview

CRM_Core_Session::getStatus() has a docblock that says it returns an array. However, it can also return NULL. This fixes that.

Before

$status initialized to NULL.

After

$status initialized to a blank string. Return type specified.

Comments

When a return type is specified, should we remove the @return from the docblock?

@civibot
Copy link

civibot bot commented Jan 26, 2023

(Standard links)

@civibot civibot bot added the master label Jan 26, 2023
@demeritcowboy
Copy link
Contributor

demeritcowboy commented Jan 26, 2023

When a return type is specified, should we remove the @return from the docblock?

No leave it in there, except in this case the docblock is wrong. It's an array (see setStatus, which behaves more like an "addStatus")

@eileenmcnaughton
Copy link
Contributor

@demeritcowboy @MegaphoneJon so this is mergable if the docblock is fixed?

@demeritcowboy
Copy link
Contributor

Yes. I may have caused some confusion by saying "leave it in there" - I meant both docblocks and typehints are useful to have, but should be in alignment.

@eileenmcnaughton
Copy link
Contributor

Thanks @demeritcowboy & yep agree

@demeritcowboy
Copy link
Contributor

@MegaphoneJon This just needs a tiny docblock update.

@demeritcowboy demeritcowboy merged commit 630bc45 into civicrm:master Feb 20, 2023
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