278016 - VersionCheck.php cleanup

Status: RESOLVED DUPLICATE of bug 287159

(addons.mozilla.org Graveyard :: Public Pages, defect)

Description

[alanjstr]

Related to bug 278014

Today, a call is made to VersionCheck for each extension and theme.  The service
should be updated to process multiple items if they are passed.

Comment 1

[(not currently active) Ted Mielczarek]

Taking, have rough patch for one possible solution.

Comment 2

[(not currently active) Ted Mielczarek]

Attachment: Simple version using comma separated ids versions, maxAppVersions

Simple backwards-compatible change that handles comma separated lists for id,
version, maxAppVersion parameters, as well as handling POST requests.

The diff sort of sucks because I reindented when I put everything in a loop. 
I'll attach a diff -w in a second.

Comment 3

[(not currently active) Ted Mielczarek]

Attachment: Same as previous, but with diff -w

More readable patch.

Comment 4

[(not currently active) Ted Mielczarek]

Attachment: html form for testing POST

You'll need to edit the action parameter to use this, obviously.

Comment 5

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 171830 [details] [diff] [review]
Simple version using comma separated ids versions, maxAppVersions

r=me

Comment 6

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

landed on branch & trunk

Comment 7

[Scott Kveton]

Have we coordinated with the Firefox devs to make sure the next version can
actually pass the information to VersionCheck in this manner?

Comment 8

[(not currently active) Ted Mielczarek]

No, but I'm looking at fixing bug 278014 myself.

Comment 9

[alanjstr]

Ok, we're gonna put it back to single item for now and discuss mutliple items later.

Comment 10

[Michael Morgan [:morgamic]]

The goal is to simplify a cut-and-dry VersionCheck.php for v1.0.  Additional
features will be held off until v2.0, where client-side planning and adequate
discussion and staging can happen before production is affected.

I will submit the VersionCheck.php patch to this bug.

Comment 11

[Michael Morgan [:morgamic]]

Attachment: Simplification of VersionCheck.php

_REQUEST no longer used, focused back on _GET.
No looping and LIMIT 1 added to an improved query for performance.
Clearer commenting and code formatting.
Removed string comparisons, improved query logic.
Simplified output.
Added debug comments.
Script dies silently.

Comment 12

[Michael Morgan [:morgamic]]

The functionality of VersionCheck.php should be added to as a part of v2.0. 
Handling multiple versions and POST requests is a great idea, but is not
supported on the client-side so it is a lot of functionality that is unnecessary
at this point.  

Therefore, for v1.0 we should have a trimmed-down VersionCheck.php that is
minimal both in performance cost as well as its scope.  Firefox does not ever
ask for multiple extensions.  Humans do not view this script.  Those two factors
alone are reasons to greatly reduce the overhead inside this script.

The first patch is an initial attempt at minimalizing VersionCheck.php - I am
working on an email to Ben regarding client-side implications.  Will update as
more information becomes available.

Should we change the summary of this bug?

Comment 13

[alanjstr]

Comment on attachment 176317 [details] [diff] [review]
Simplification of VersionCheck.php

>+// Map the mysql main.type enum into the right type.
> $ext_typemap = array('T' => 'theme',
>                      'E' => 'extension',
>                      'P' => 'plugin');

We should have this in a global file, eventually, since we use it so much.

>+    /*   Deprecated    */
>+    // $reqItemMaxAppVersion = explode(",",mysql_real_escape_string($_GET['maxAppVersion']));
>+
>+

If we're not using this I think we can just delete it.

+	     '{$reqTargetAppVersion}+' >= version.minappver AND

Why the plus sign?  

+	     '{$reqItemVersion}' < version.version

This only returns something if a new version available.  But what if the
current version had its maxVer raised.	For example:

Assume that you just updated your browser from Firefox 1.0 to Firefox
1.1.  The install.rdf of your extension says that it is only
compatible through Firefox 1.0, so the installer for 1.1 disables it.
The call to VersionCheck returns that the maxVer of FooFoo's
compatibility is now 1.1, so Firefox can re-enable it.	In theory,
this check would happen during the installation of Firefox 1.1 so that
it doesn't have to be disabled in the first place.

>+        ORDER BY
>+            extversion DESC
>+        #    version.MaxAppVer_int DESC,
>+        #    version.version DESC,
>+        #    version.osid DESC

We need to sort by internal application version first, ie Firefox 1.0PR has an
internal version of 0.10 vs Firefox 1.0 has an internal version of 1.0.  Also,
we need to sort by OS ID Desc so that we get an OS Specific version before the
OS All.  An example would be Pinball 0.9.9.2 for OSX.  This is to overcome the
UMO limitation that ver 1-1 os


>+                $output  = "<?xml version=\"1.0\"?>\n";
>+                $output .= "<RDF:RDF xmlns:RDF=\"http://www.w3.org/1999/02/22-rdf-syntax-ns#\" xmlns:em=\"http://www.mozilla.org/2004/em-rdf#\">\n\n";

Can we get a content type header for this?

>+    echo '<head>';
>+    echo '<title>VersionCheck.php Debug Information</title>';

Can we get a content type header for this?

Comment 14

[alanjstr]

morgamic -
The usual method for undoing a change is to open a new bug.  This bug, and the
bug it blocks, are about multiple items in one pass.  

Comment 15

[Michael Morgan [:morgamic]]

(In reply to comment #13)
> +	     '{$reqTargetAppVersion}+' >= version.minappver AND
> 
> Why the plus sign?  

Without the plus-sign, the query will eliminate items unnecessarily from the
range if they don't have one.   It has to do with how MySQL does string
comparisons.  For example, 1.0 is >= 1.0+, but not as far as strings are
concerned.  So we tack it on.
 
> +	     '{$reqItemVersion}' < version.version
> 
> This only returns something if a new version available.  But what if the
> current version had its maxVer raised.	For example:
> 
> Assume that you just updated your browser from Firefox 1.0 to Firefox
> 1.1.  The install.rdf of your extension says that it is only
> compatible through Firefox 1.0, so the installer for 1.1 disables it.
> The call to VersionCheck returns that the maxVer of FooFoo's
> compatibility is now 1.1, so Firefox can re-enable it.	In theory,
> this check would happen during the installation of Firefox 1.1 so that
> it doesn't have to be disabled in the first place.
> 
> >+        ORDER BY
> >+            extversion DESC
> >+        #    version.MaxAppVer_int DESC,
> >+        #    version.version DESC,
> >+        #    version.osid DESC
> 
> We need to sort by internal application version first, ie Firefox 1.0PR has an
> internal version of 0.10 vs Firefox 1.0 has an internal version of 1.0.  Also,
> we need to sort by OS ID Desc so that we get an OS Specific version before the
> OS All.  An example would be Pinball 0.9.9.2 for OSX.  This is to overcome the
> UMO limitation that ver 1-1 os

I've adjusted the queries in the next patch; see if that works better.

> 
> >+                $output  = "<?xml version=\"1.0\"?>\n";
> >+                $output .= "<RDF:RDF
xmlns:RDF=\"http://www.w3.org/1999/02/22-rdf-syntax-ns#\"
xmlns:em=\"http://www.mozilla.org/2004/em-rdf#\">\n\n";
> 
> Can we get a content type header for this?

The content type header is displayed with the $output.  The quoted code is just
generating the output and storing it.  The content type is set when the data is
actually sent via an 'echo'.

> >+    echo '<head>';
> >+    echo '<title>VersionCheck.php Debug Information</title>';
> 
> Can we get a content type header for this?

I'm pretty sure that text/html should be the default for .php documents.

Comment 16

[Michael Morgan [:morgamic]]

Attachment: Query improvements, additional debugging, urldecode

Updated query based on alanjstr's comments.  
Behavior adjusted to include current version information so Firefox can update
metadata for given item.
Additional debugging.
Added urldecode and trim for all required arguments.
Re-added compat bits for Firefox 0.9.

Comment 17

[Giorgio Maone [:ma1]]

Comment on attachment 176659 [details] [diff] [review]
Query improvements, additional debugging, urldecode

Output seems OK. Code looks good. Negative review mainly because of the
urldecode() applied to $_GET vars, which are already decoded by PHP runtime. We
probably don't need trim() either, since parameters are generated by the
browser itself, so I guess we can cut that "else" branch. Why the brackets
around $os_id in the ternary assignment to $os_query?  You should also consider
using heredoc syntax when building XML output: it would improve a lot
readability (all those echo "..." are simply ugly!). You've done a great work
with SQL formatting, instead :-)

Comment 18

[Michael Morgan [:morgamic]]

This is fixed in the config patch, which contains a VersionCheck.php that
addresses Mao's concerns. In the last comment.  Features for this script should
be discussed further for v2.0.  Until then, as we've said, it should remain as
simple and predictable as possible.

*** This bug has been marked as a duplicate of 287159 ***

Comment 19

[Michael Morgan [:morgamic]]

AMO BUGSPAM FOR COMPONENT MOVE AND DELETE (FILTER ME)