#2026 Wrong Doc Blocks #2027
#2026 Wrong Doc Blocks #2027
Conversation
Fixed adminfunction_templaets and mysql(i)
Fixed the database classes
Fixed functions.php
Finished "inc"
|
I've changed all doc blocks in "inc" and "admin/inc". Though there are also some functions in other files those are the important ones. @euantorano |
| { | ||
| global $shutdown_queries; | ||
| if($name) | ||
| if(!empty($name)) |
There was a problem hiding this comment.
Any reason you used !empty() here (and in at least 2 more places)? It's a parameter so it's always set, right?
There was a problem hiding this comment.
empty is the same as == "". Doesn't really matter, it's more a personal preference - looks nicer to me.
There was a problem hiding this comment.
!empty() negation also checks isset() first which is not needed here. So it's a little overkill. Doesn't almost matter here, I know, but it's not really worth changing.
There was a problem hiding this comment.
After a quick look most of these cases should be changed to trim($var) == false. Is that ok to you?
There was a problem hiding this comment.
I don't think it was forced for the name to be a string so I wouldn't go with trim, empty seems fine.
There was a problem hiding this comment.
@Destroy666x ? Otherwise I'd like to get this merged ASAP as this PR will likely get conflicts soon and those are a PITA to fix here ;)
There was a problem hiding this comment.
Somebody is going to get a PITA fixing conflicts though..:p
There was a problem hiding this comment.
A lot of the other PRs don't change doc blocks, they shouldn't get a problem. But I made the experience that git has problems when you change a lot of a file and someone modifies the file in the meantime.
There was a problem hiding this comment.
Well, I see no reason to not just stay with if($name) as I said. It checked what it was supposed to check - whether the parameter contains any value. Nothing else needs to be done here.
Also, is this PR finished? I thought it's still WIP as the title says, if I'm not mistaken not all files were changed as you mentioned:
I've changed all doc blocks in "inc" and "admin/inc". Though there are also some functions in other files those are the important ones.
There aren't many functions outside of these folders so would be nice to fix their comments too.
There was a problem hiding this comment.
I still think that at least the trim version should be used but well, changed everything. This isn't WIP anymore
Removes some "will be fetched" notes from "format_name"
Fixes prefix cache issues
Went through the root files
Fixed the function in "install"
Fixed archive files
Fixed admin directory
Remove empty checks
| @@ -682,6 +697,14 @@ function whatsnext() | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * Determine the nect function we need to call | |||
| * @param string $name The cookie identifier. | ||
| * @param string $value The cookie value. | ||
| * @param int|string $expires The timestamp of the expiry date. | ||
| * @param boolean $httponly True if setting a HttpOnly cookie (supported by IE, Opera 9, Konqueror) |
There was a problem hiding this comment.
I'd change it to (supported by the majority of web browsers)
|
What was changed looks and seems to work fine to me (except the 3 small change comments). So I guess this can be merged. But keep in mind that there still may be mistakes, for example just corrected several wrong return values and missing @return here: https://github.com/mybb/mybb/pull/2052/files |
|
Yep, this is a pretty major changeset, so I wouldn't be surprised if there are a few small typos. If we can fix the ones noted above, we can merge this (which would be super awesome). |
|
Well, I haven't looked at the complete functions, otherwise I would still fix issues in a few years :P Will look at the three changes tomorrow and merge this afterwards if noone else mentions anything till then. |
Fixed issues @Destroy666x mentioned
|
Fixed and merged |
#2026
Please only comment on the files I've already edited - I haven't looked at the others so comments about others will be ignored anyways.
Also note that I've only tried to fix the doc blocks and haven't changed variables where eg 0/1 is used as boolean switch.