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
Add return flags to breadcrumbs and you are here to allow later processing #2251
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
global $lang; | ||
global $conf; | ||
|
||
//check if enabled | ||
if(!$conf['breadcrumbs']) return false; | ||
|
||
//set default | ||
if(is_null($sep)) $sep = '•'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the change from the default value in the signature to implementing it in code here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the PR description:
The functions had a default separator defined in the signature, but this has been moved into the function body to allow skipping this with the value NULL so that the $return flag can be set without overriding the separator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but isn't that exactly the behavior if the default value is set in the signature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it doesn't work when we need to skip the first argument. It has to include that one more line: https://stackoverflow.com/questions/9166914/using-default-arguments-in-a-function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right! Sorry for reading it only superficially :/
I found myself needing to process the output of the two breadcrumb functions, so I've added a return flag to the functions so that they can be stored in a variable, rather than printing to the browser immediately.
The functions had a default separator defined in the signature, but this has been moved into the function body to allow skipping this with the value NULL so that the $return flag can be set without overriding the separator.