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

Web: fix deprecated php stuff #2971

Merged
merged 3 commits into from
Mar 18, 2019
Merged

Conversation

ChristianBeer
Copy link
Member

Description of the Change

  • The ereg function is deprecated since 5.3 and was removed in 7.0.
  • Using break; outside a loop was working unintentionally before 7.0 but now throws a compile error.
  • Using s call-time pass-by-reference throws an error since 5.4. The & is only needed in the function declaration not when calling the function.

Fixes #2102

Release Notes
N/A

The `ereg` function is deprecated since 5.3 and was removed in 7.0.
Using `break;` outside a loop was working unintentionally before 7.0 but now throws a compile error.
Using s call-time pass-by-reference throws an error since 5.4. The `&` is only needed in the function declaration not when calling the function.
@davidpanderson
Copy link
Contributor

Also, in translation.inc could change
$text = strtr($text, $replacements);
return $text;
to
return strtr($text, $replacements);

@ChristianBeer
Copy link
Member Author

This PR is focused on deprecated PHP stuff not simplifying the existing code. Adding unrelated changes is the equivalent to the "Feature Creep" Antipattern.

@davidpanderson
Copy link
Contributor

those lines produced a warning under php 7

@@ -326,7 +326,7 @@ function show_next($iter, $view) {
//
show_refresh_finished();
$refresh->update('count=count+1');
break;
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

I have no full picture of what PHP's quirky use of break outside a loop was doing in different contexts, nor do I know what it was meant to do in this context. AFAIK, break outside a loop exits a script: what does that say about using it in a function like this? Yes, it could mean return but I don't know.

Bottom line: this should be reviewed by David who knows the intended semantic of this code.

Otherwise I'm fine with the changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

@davidpanderson Can you recall why you put a break here? I don't have the means to test this part.

Copy link
Contributor

Choose a reason for hiding this comment

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

That break would have behaved the same as return in this case (no PHPun intended). Both calls to show_next() are made inside of case statements and are immediately followed by a break, so if execution was reaching that drifter of a break inside the function, and this was not generating a fatal runtime error about break being called out of scope, it would just cause execution to break out of the parent switch in the calling routine (which it was going to do immediately, anyway, upon return). No need to hold up this PR, in my opinion.

@brevilo
Copy link
Contributor

brevilo commented Jan 21, 2019

those lines produced a warning under php 7

I'm with David on this one, if it's really a warning (although I'd consider this an optimization).

@Rytiss
Copy link
Contributor

Rytiss commented Jan 22, 2019

Which exact warning does PHP throw in this case? Assigning to var and returning later looks like a very common pattern.

@ChristianBeer
Copy link
Member Author

I didn't reproduce the case David posted but it was not listed in Scrutinizer as an Issue. I nevertheless changed it as I was fixing the other issues Scrutiniter showed in this file. I didn't check other files.

@davidpanderson
Copy link
Contributor

Per Oliver:
FILE: html/inc/translation.inc

FOUND 1 ERROR AFFECTING 1 LINE

196 | ERROR | Since PHP 7.0, functions inspecting arguments, like func_get_arg(), no longer report the original value as passed to a parameter, but will instead provide the current value. The parameter "$text" was changed on line 184.

@ChristianBeer ChristianBeer changed the title Web: fix deprecated php stuff [WIP] Web: fix deprecated php stuff Jan 23, 2019
@ChristianBeer
Copy link
Member Author

I already explained why the func_get_arg() call is fine in #2102 (comment). Since the two lines at the end of the function don't produce an error or warning, I'm going to change the commit again and force push a new one that states that it is an optimization not a fix. The current commit message is misleading.

@ChristianBeer ChristianBeer changed the title [WIP] Web: fix deprecated php stuff Web: fix deprecated php stuff Jan 23, 2019
@ChristianBeer
Copy link
Member Author

This is ready for review again. As a side note I'm currently fiddling with the Scrutinizer config to hide all the unrelated issues and then plan on having a look at what remains.

Copy link
Contributor

@brevilo brevilo left a comment

Choose a reason for hiding this comment

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

I'm approving all changes but the one I commented on earlier. That still needs to be approved by @davidpanderson .

@ChristianBeer
Copy link
Member Author

This is some basic stuff. Could someone please merge this?

@davidpanderson davidpanderson merged commit e65a6b1 into master Mar 18, 2019
@AenBleidd AenBleidd deleted the cb_fix_deprecated_php_stuff branch March 21, 2019 19:25
@AenBleidd AenBleidd added this to the Server Release 1.2.0 milestone Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrading from PHP 5 to PHP 7
6 participants