1083111 - PHP 4 => PHP5 update of the code

Status: RESOLVED WONTFIX

(www.mozilla.org :: Product Details, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

Attachment: php5.diff

This does not remove or change anything in the behavior of the library.

Comment 1

[Pascal Chevrel:pascalc]

Comment on attachment 8505412 [details] [diff] [review]
php5.diff

Review of attachment 8505412 [details] [diff] [review]:
-----------------------------------------------------------------

Generally speaking there would be more to do to make it really PHP 5 code but these changes are OK. One big missing piece is that classes shoud never be in the global namespace, but adding a namespace to these classes is likely to break users of the class. A lot of methods could probably just be deleted as their role was to output download buttons. The one file that is really in use, export.json, could be cleaned up and simplified as it has quite a lot of duplicated code. That said, since these changes are an improvement, r+ from me :)

::: export_json.php
@@ +32,4 @@
>  /** Product Details */
>  // Firefox Versions
>  require_once 'firefoxDetails.class.php';
> +$fx_constants = ['LATEST_FIREFOX_VERSION',

You should put the constant names on a separate line and not indent them so much (indent 4 spaces)

@@ +107,4 @@
>  
>  // Firefox Primary Builds
>  $tbd = new thunderbirdDetails();
> +$builds = ['primary_builds', 'beta_builds'];

This array is already defined line 85 and can be removed

@@ +129,4 @@
>      'builds'  => mobileDetails::primary_builds(false),
>      'beta_builds' => mobileDetails::beta_builds(false),
>      'alpha_builds' => mobileDetails::alpha_builds(false),
> +    ];

nit, don't indent the closing bracket

@@ +134,5 @@
>  
>  // Mobile History
>  require_once 'history/mobileHistory.class.php';
>  $mobh = new mobileHistory();
> +$releases = ['major_releases', 'stability_releases', 'development_releases'];

already defined line 118, can be removed

::: firefoxDetails.class.php
@@ +866,5 @@
>          /**
>           * Constructor.
>           */
> +        public function __construct() {
> +            parent::__construct();

The parent constructor is already automatically loaded in an extended class, since there is nothing different added in the extended class constructor, a static call to the parent constructor seems to be unnecessary added complexity. It could be that in PHP 4 this was needed, but I think it can be safely remoced today. (this comment is true for other constructors of extended classes in your patch that just call the parent constructor)

@@ +878,5 @@
>           *      devel_version: boolean, return a development release.  Default: false
>           * @return string HTML block
>           */
> +        function getAncillaryLinksForLocale($locale, $options=[]) {
> +            $devel_version            = array_key_exists('devel_version', $options) ? $options['devel_version'] : false;

indentation before the space can be removed

@@ +1130,4 @@
>              $_product         = array_key_exists('product', $options) ? $options['product'] : 'firefox';
>              $_latest_version  = array_key_exists('latest_version', $options) ? $options['latest_version'] : LATEST_FIREFOX_VERSION;
>              $_product_version = array_key_exists('product_version', $options) ?  $options['product_version'] : 'newest';
> +            $_product_version = in_array($_product_version, ['newest', 'devel', 'oldest']) ? $_product_version : 'newest';

We could use a closure since we now do PHP 5, so as to avoid potential error like a typo in all of the ternary tests and also for better readability:

$get_from_options = function($needle, $fallback) use($options) {
    return isset($options[$needle]) ? $options[$needle] : $fallback;
};

$_product         = $get_from_options('product', 'firefox');
$_latest_version  = $get_from_options('latest_version', LATEST_FIREFOX_VERSION);
$_product_version = $get_from_options('product_version', 'newest');

$_product_version = in_array($_product_version, ['newest', 'devel', 'oldest']) 
                    ? $_product_version 
                    : 'newest';

Comment 2

[Sylvestre Ledru [:Sylvestre]]

(In reply to Pascal Chevrel:pascalc from comment #1)
> Comment on attachment 8505412 [details] [diff] [review]
> php5.diff
> 
> Review of attachment 8505412 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Generally speaking there would be more to do to make it really PHP 5 code
> but these changes are OK.
Yes, that is a first step. ;)

Merged with your comment (thanks):
http://viewvc.svn.mozilla.org/vc?view=revision&revision=133397

Comment 3

[Josh Mize [:jgmize]]

This appears to have broken www-dev.allizom.org (see https://bugzilla.mozilla.org/show_bug.cgi?id=1086665), and will break www.mozilla.org if we do a production push that updates product-details. Can we please revert this ASAP?

Comment 4

[Pascal Chevrel:pascalc]

change reverted in r133397, jakem informed that we use php-5.3.3-38.el6.x86_64 on RHEL and not 5.4 as we thought.

Comment 5

[Pascal Chevrel:pascalc]

bad copy paste, the revert number is 133447

Comment 6

[Josh Mize [:jgmize]]

Thanks :pascalc!

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Touching product details is too risky. We will work on bug 1083718 instead.