287159 - UMO Configuration is hard to manage

Status: RESOLVED FIXED

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

Description

[Michael Morgan [:morgamic]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6) Gecko/20050223 Firefox/1.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6) Gecko/20050223 Firefox/1.0.1

UMO is hard to configure, does not adequately define its constants, and mixes
config with initialization data.  This should be improved in order to serve as a
foundation for distributed development.

Reproducible: Always

Comment 1

[Michael Morgan [:morgamic]]

Attachment: Config patch to improve overall UMO config.

This patch covers almost every document and deals mainly with defining
constants and organizing config / init data properly.

Comment 2

[alanjstr]

A few comments:
1) This is a huge patch.  By putting it all into one patch, its probably a
little harder to review.  However, it is of course reviewer's choice.
2) At a cursory glance, "/developers/" is still hardcoded.  This isn't
localization friendly and also means that changing it has to hit multiple files
(such as -tmp).  I'm not saying that needs to be addressed right now, probably
as part of the rewrite.

That said, I'm glad to see this bug and patch.  Its ok to assign bugs to
yourself if you're going to work them.

Comment 3

[Scott Kveton]

I asked Mike to submit this large patch on IRC.  This is going to help us moving
forward with the security audit.

Comment 4

[Colin Ogilvie [:cso]]

Assuming this patch is meant to convert all urls to be dependant on the config
variables, it misses one in mail_newaccount.php

Comment 5

[alanjstr]

Comment on attachment 178200 [details] [diff] [review]
Config patch to improve overall UMO config.

Please keep in mind that many of these are general comments about the code and
not specific to this page.


>Index: index.php
>===================================================================
>-<?php
>+// security update ???
> $securitywarning=false;
> if ($securitywarning=="true") {
> ?>

If we have an important message to display on the front page, this would be
theoretically be database driven instead of popping in a new front page.  

> 
>-    <?php
>-    $uriparams_skip="application";
>-    ?>

Actually, we need this.  Take a look at how the URLs are constructed

for: <a href="/extensions/?<?php echo"".uriparams()."&amp;";
?>application=firefox">Firefox</a>, <a href="/extensions/?<?php 

Notice that if you don't skip application, you'll wind up having it twice.

> 	</dl>
>-    <?php
>-    unset($uriparams_skip);
>-    ?>

Of course you have to turn it back on for all the other URLs on the page.

>+        // Took out the compatibility stuff to avoid a blank front page.
>+        // `minAppVer_int` <='$currentver' AND `maxAppVer_int` >= '$currentver' AND 

Actually, $currentver is the current version of the application we've detected
http://lxr.mozilla.org/mozilla/source/webtools/update/index.php#53


>-include"$page_footer";
>+require_once(FOOTER);

>+require_once './core/init.php';

This isnt' really consistent.  

> if ($_GET["type"]=="E") {
>     $type="extensions";
> } else if ($_GET["type"]=="T") {
>     $type="themes";
> }

What happens if type is neither?

$return_path="$type/moreinfo.php?id=$id&vid=$vid&".uriparams()."&page=comments&
pageid=$_GET[pageid]#$commentid";

You left a GET in the middle of this URI

>-header("Location: http://$sitehostname/$return_path");
>+header('Location: http://'.HOST_NAME.'/'.$return_path);

Are we increasing overhead by doing http and not https here?

>+ $uri = str_replace(REPO_PATH.'/approval/','http://'.HOST_NAME.'/developers/approvalfile.php/',$newpath);

Can we get some whitespace after the commas in this one?  


>-global $connection, $sitehostname, $ftpurl, $repositorypath;
>+global $connection;

Non-coder question: do these need to be defined as globals here?

>+    $message .= 'http://'.HOST_NAME.'/developers/createaccount.php?function=confirmaccount&email=$email&confirmationcode=$confirmationcode'."\n\n";

This should be https.  Now that I think of it, don't we always preface
HOST_NAME with https?  In which case, shouldn't the variable just include it?

>-    $file = "$websitepath/$file";
>+    $file = FILE_PATH.'/'.$file;

Is there a reason we're not putting these previews in their own subdirectory?  

>+    $page_headers = '<link rel="alternate" type="application/rss+xml"
>+        title="RSS" href="http://'.HOST_NAME.'/'.$rssfeed.'>';

> //Icon Bar Modules
> echo"<DIV style=\"margin-top: 30px; height: 34px\">";
> echo"<DIV class=\"iconbar\">";
> if ($appname=="Thunderbird") {
>-    echo"<A HREF=\"moreinfo.php?".uriparams()."&amp;id=$id\"><IMG SRC=\"/images/download.png\" HEIGHT=32 WIDTH=32 TITLE=\"More Info about $name\" ALT=\"\">More Info</A>";
>+    echo"<A HREF=\"moreinfo.php?".uriparams()."&amp;id=$id\"><IMG SRC=\"../images/download.png\" HEIGHT=32 WIDTH=32 TITLE=\"More Info about $name\" ALT=\"\">More Info</A>";

This is the beginning of a lot of escaped html.  All that escaping makes it a
lot harder to read.

-header("Content-Type: application/xml; charset=utf-8");
+header("Content-Type: text/xml; charset=utf-8");

Did you notice that above you did type="application/rss+xml"?  Also, RDF should
be served as application/rdf+xml

+    $title = $titles[$_GET['page']];
+$page_title = 'Mozilla Update :: Themes -- More Info: '.$row['Name']; 
+if ($title) {
+    $page_title .= ' - '.$pagetitle;
+}


That doesn't look right to me.	Shouldn't it be
if ($title) {
    $page_title .= ' - '.$title;
}

mozilla/webtools/update/update/VersionCheck.php 

Isn't this a separate patch that got R- from mao already?

As you mentioned to me on IRC, there are concerns about various paths.	I'd
rather see them defined once, even if its just appending "/ftp/" or some such.

Comment 6

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

I gave it a cursory look and it seems ok, but I don't have nearly enough time to
carefully review this.  I'm all for checking it in and finding bugs later.

Comment 7

[Giorgio Maone [:ma1]]

Comment on attachment 178200 [details] [diff] [review]
Config patch to improve overall UMO config.

If you've time enough, please check alanjstr's comment #5. Otherwise, to get
"+", you just need replacing $pagetitle with $title in

$page_title .= ' '.$pagetitle;

and doublechecking the $uriparams_skip issue.

Comment 8

[Michael Morgan [:morgamic]]

*** Bug 278016 has been marked as a duplicate of this bug. ***

Comment 9

[Michael Morgan [:morgamic]]

Attachment: Revised config patch.

Fixed errors in developers/, VersionCheck, and adjusted code based on feedback.

Comment 10

[Michael Morgan [:morgamic]]

Attachment: Bonehead fix - left urldecode() in by accident..

Comment 11

[Michael Morgan [:morgamic]]

Comment on attachment 179658 [details] [diff] [review]
Bonehead fix - left urldecode() in by accident..

This has been checked in; we will find the rest of the errors during testing -
April 5th is around the corner.

Comment 12

[Michael Morgan [:morgamic]]

We will find config errors during testing.  So we can progress, this is resolved
-- handle small bugs and errors in groups during testing and note any
adjustments on your commit logs.

Comment 13

[Giorgio Maone [:ma1]]

Comment on attachment 179658 [details] [diff] [review]
Bonehead fix - left urldecode() in by accident..

ok, let's cross our fingers :)