MozillaWiki

Update:Archive/1.0/Developers: Difference between revisions

Update Discussion

Update:Archive/1.0/Developers (view source)

Revision as of 00:49, 29 April 2007

6,242 bytes added ,  29 April 2007
no edit summary
No edit summary
 
(43 intermediate revisions by 4 users not shown)
Line 1: Line 1:
[[Update:Home_Page|Update Home Page]] » [[Update:Development|Development]] » Audit
{{AmoArchive}}


== Introduction ==
== Introduction ==
Line 222: Line 222:


=== Planning and Preparation ===
=== Planning and Preparation ===
* Eliminate areas that will not ever be implemented (until v2.0)
* <strike>Eliminate areas that will not ever be implemented (until v2.0)</strike>
* Determine which sections of UMO need the most attention
* <strike>Determine which sections of UMO need the most attention</strike>
* Create list of items we will review
* <strike>Create list of items we will review</strike>
* Clean code for review (PHP_beautifier)
* <strike>Clean code for review (PHP_beautifier)</strike> - did not make a difference, moved on.


=== Code Review ===
=== Code Review ===
* Document includes, dependencies
* <strike>Document includes, dependencies</strike>
* Review application based on priorities, listing needed changes
* <strike>Review application based on priorities, listing needed changes</strike>
** Imperative security changes
** <strike>Imperative security changes</strike>
** Efficiency issues that really need to be changed
** <strike>Efficiency issues that really need to be changed</strike>
** Efficiency or style issues that are cosmetic
** <strike>Efficiency or style issues that are cosmetic</strike>
* Wiki might be good for cataloguing these
* Test behavior in identical staging environment for all public use-cases. (morgamic, chenb)


=== Code Adjustments ===
=== Code Adjustments ===
* Update code based on quantified 'needed changes'
* <strike>[https://bugzilla.mozilla.org/show_bug.cgi?id=283215 Patch branch to include local modifications in prod.] (under review)</strike>
* If possible, update code for readability and efficiency
* Patching
** <strike>Rewrite VersionCheck.php</strike> (morgamic)
*** <strike>This is currently being reviewed. (mconnor)</strike>
** <strike>Update UMO configuration and structure to make development easier.</strike> (morgamic)
*** <strike>Stripping out stuff like hard-coded paths</strike>
*** <strike>Adding needed directories to CVS</strike>
*** Documenting requirements and installation procedure in Wiki (need help with this)
** <strike>Get PFS into CVS (morgamic, jst, kveton)</strike>
*** <strike>uniqueUrl.php needs to be excluded</strike>
** <strike>Analyze</strike>, optimize PFS scripts. (morgamic, pending import being cleared by jst)
** <strike>Prioritize needed work and create TODO lists.  (morgamic, alanjstr, pending patches)</strike> -- see hitlist below
 
** Deal out important patching (based on audit log) to community based on wiki information. (kveton, morgamic,alanjstr)
** <strike>Unfreeze branch and open it up for patching. (kveton)</strike>
 
=== Hit list ===
# Developer Sessions, making sure login works correctly, nav options for admins are hidden (morgamic)
# Comment spam prevention for user ratings, reenabling comments for extensions and themes (chip)
# <strike>Set up -dev,-staging to automatically pull from branch, trunk(kveton)</strike>
# QA for submitted patches (Ctho, alanjstr)
# Run through remaining items in audit log that are important mainly (but not limited to):
## missing input validation
## bad loops
# Work through workflows for developers and admins (everyone)
# Merge branch with trunk (kveton)
# Push trunk into production (justdave,kveton)


=== Assessment ===
=== Assessment ===
* Benchmark code to assure scalability
* Benchmark code to assure scalability
* Check for improvements on performance
* Check for improvements on performance
== Audit CVS Information ==
Line numbers mentioned for the files below may be relative.  In order to prevent confusion, the output of 'cvs status' was captured in case they do not match.
View the [[Update:Development:v1.0:CVS_Info|CVS information page]] for the complete 'cvs status' of the audited code.


== Audit Log ==
== Audit Log ==
The following is a list of all .php files in UMO.  For each file the following will be determined:
The following is a list of all .php files in UMO.  For each file the following will be determined:
* Is the file live?
* Is the file live?
Line 254: Line 283:
** Semi-urgent performance changes - things that could make a big difference in performance
** Semi-urgent performance changes - things that could make a big difference in performance
** Best-case scenario changes - things that probably _should_ be tended to but could wait
** Best-case scenario changes - things that probably _should_ be tended to but could wait
----
==== CVS Information ====
Line numbers mentioned for the files below may be relative.  In order to prevent confusion, the output of 'cvs status' was captured in case they do not match.


View the [[Update:Development:v1.0:CVS_Info|CVS information page]] for the complete 'cvs status' of the audited code.
----


==== General Notes ====
=== General Notes ===
----
----
* use single quotes for associative array key strings, e.g. $_GET['id'] instead of $_GET[id]
* use single quotes for associative array key strings, e.g. $_GET['id'] instead of $_GET[id]
Line 270: Line 294:


* [https://bugzilla.mozilla.org/buglist.cgi?product=Update&chfieldfrom=2005-01-01&chfieldto=Now&chfield=%5BBug+creation%5D&chfield=resolution&query_based_on=UMO+1.1&field0-0-0=bug_group&type0-0-0=equals&value0-0-0=update-security Security Bugs]
* [https://bugzilla.mozilla.org/buglist.cgi?product=Update&chfieldfrom=2005-01-01&chfieldto=Now&chfield=%5BBug+creation%5D&chfield=resolution&query_based_on=UMO+1.1&field0-0-0=bug_group&type0-0-0=equals&value0-0-0=update-security Security Bugs]
* usage of uriparams() is done even if it doesn't make any sense


===/.===
===/.===
Line 279: Line 304:


====/quicksearch.php====
====/quicksearch.php====
[https://bugzilla.mozilla.org/show_bug.cgi?id=278940 Make update's search feature not suck]
----
----


Line 302: Line 328:
* [44] [64-66] use include_once
* [44] [64-66] use include_once
* why is there a function defined in this file? it's a "config" file
* why is there a function defined in this file? it's a "config" file
* This config file should use define() for configuration constants, or at least use a $config[] array so that any configuration variables are clearly marked when used during runtime in a script.
* That said, this file should only define constants and config variables.  It should not be performing any tasks.  The act of including things like browserdetect should be done by an init script.  This allows us to include site configuration information without having to run anything (something we could use in the RDF generators for PFS, VersionCheck).


====/core/dbconfig.php====
====/core/dbconfig.php====
* [48] $db (a boolean) is assigned to the returned value of mysql_select_db, but never used anywhere
* [48] $db (a boolean) is assigned to the returned value of mysql_select_db, but never used anywhere
* This file is pointless.  DB information should just be defined in config.php. (alanjstr says: the problem would be the includes at the bottom of core/config.php.  VersionCheck doesn't need all of that)
* This file should not create the connection.  Connections should only be created when they will be used.  As it stands, this forces every page on UMO to create a DB connection whether or not it will be used for anything.
* Someone could use a script, point at this file and create however many unused DB connections they wanted by simply opening it.
* The proper way to handle db connections would be to explicitly connect to the db in the script that is doing a query.  It reduces confusion and also allows for scripts that don't connect just because they include the config/init scripts.


====/core/inc_browserdetection.php====
====/core/inc_browserdetection.php====
Line 310: Line 342:


====/core/inc_footer.php====
====/core/inc_footer.php====
* [https://bugzilla.mozilla.org/show_bug.cgi?id=272806 Use Templates]
* ending body and html tags should be in footer document instead of having them randomly scattered throughout the application.
* ending body and html tags should be in footer document instead of having them randomly scattered throughout the application.


====/core/inc_global.php====
====/core/inc_global.php====
* [74] [79] can stripslashes() be used in place of str_replace()?
* [74] [79] can stripslashes() be used in place of str_replace()? (alanjstr says: we need to come up with a list of when to strip and what to strip.  This is different for php variables and database entry)
* strip_tags is not 100% safe; it can be fooled. Perhaps consider kses
* strip_tags is not 100% safe; it can be fooled. Perhaps consider kses
* [86] strtolower() is unnecessary
* [86] strtolower() is unnecessary
Line 410: Line 443:


====/developers/createaccount.php====
====/developers/createaccount.php====
* [https://bugzilla.mozilla.org/show_bug.cgi?id=275913 Bug 275913]


====/developers/faqmanager.php====
====/developers/faqmanager.php====
Line 438: Line 472:


====/developers/listmanager.php====
====/developers/listmanager.php====
* [https://bugzilla.mozilla.org/show_bug.cgi?id=282846 List Manager for Themes links to Add Item for Extensions]
* [27] what if $_GET['type'] is some random nonempty string?
* [27] what if $_GET['type'] is some random nonempty string?
* [30] [55] [58] [99] [153] [410] [422] [423] [430] [556] [557] Use single quotes for the associative array keys
* [30] [55] [58] [99] [153] [410] [422] [423] [430] [556] [557] Use single quotes for the associative array keys
Line 444: Line 479:
* [175-184] nested loop with queries that can be rewritten to reduce the number of queries to two
* [175-184] nested loop with queries that can be rewritten to reduce the number of queries to two
* [523] this variable is not used anywhere after the increment
* [523] this variable is not used anywhere after the increment
----
alanjstr suggests:
* [https://bugzilla.mozilla.org/show_bug.cgi?id=282845 Replace List Manager with simple search]


====/developers/login.php====
====/developers/login.php====
Line 460: Line 498:


====/developers/previews.php====
====/developers/previews.php====
* [https://bugzilla.mozilla.org/show_bug.cgi?id=275157 Bug 275157]
* [46] this variable is not used anywhere
* [46] this variable is not used anywhere
* [197-202] consider replace these nested if elses with a switch statement; the code that follows doesn't concern with the status, but it should.
* [197-202] consider replace these nested if elses with a switch statement; the code that follows doesn't concern with the status, but it should.
Line 469: Line 508:


====/developers/usermanager.php====
====/developers/usermanager.php====
----
* [https://bugzilla.mozilla.org/show_bug.cgi?id=281273 Deleting an author strands addons]
* [https://bugzilla.mozilla.org/show_bug.cgi?id=281273 Deleting an author strands addons]
* [https://bugzilla.mozilla.org/show_bug.cgi?id=275137 Email address changes are not verified]
* [https://bugzilla.mozilla.org/show_bug.cgi?id=275137 Email address changes are not verified]
* [153] [179] [188] [307] [318] [414] [475] [504] should check for affected rows
* [160-172] [194-206] loop with queries, consider rewrite
* [245-248] consider replacing nested if elses with switch statement
----
----
alanjstr suggests:
alanjstr suggests:
* [https://bugzilla.mozilla.org/show_bug.cgi?id=279524 Break this into multiple pages.  Ditch the mass user management entirely.]
* [https://bugzilla.mozilla.org/show_bug.cgi?id=279524 Break this into multiple pages.  Ditch the mass user management entirely.]
----
* [153] [179] [188] [307] [318] [414] [475] [504] should check for affected rows
* [160-172] [194-206] loop with queries, consider rewrite
* [245-248] consider replacing nested if elses with switch statement


===/extensions===
===/extensions===
* These files are duplicated in /themes.  They should really be abstracted and moved up one directory.  Otherwise, any changes happening in /themes will have to be re-implemented in /extensions and vice-versa.
* These files should all be migrated to reduce code:
** /extensions/authorprofiles.php -> ../authorprofiles.php
** /extensions/inc_sidebar.php -> ../core/[something]
** /extensions/index.php -> ../extensions.php
** /extensions/moreinfo.php -> ../moreinfo.php
** /extensions/showlist.php -> ../showlist.php
====/extensions/authorprofiles.php====
====/extensions/authorprofiles.php====
* [98] should htmlentities($userwebsite) for the href property
* [98] should htmlentities($userwebsite) for the href property
Line 501: Line 546:


===/plugins===
===/plugins===
* In general, it's not in CVS which is bad.
** We know what part can't be in CVS (uniqueUrl.php), so it will be committed soon.
* Why isn't this information in a database?
** If there was a mime-type to file data map along with some mappings for compatibility,  this could be a much simpler script.
** On that note, why can't the existing UMO database be used with the addition of a mime-type table and a mime-type to version map?
** If a query with LIMIT 1 could happen, the overhead for this script would be reduced because there wouldn't be so many preg_match calls and string comparisons, which gets pretty expensive when you've got them in a series of if-else's.
* How do people update their plug-ins?  Can they?  (doesn't look like it)
* Logging is going to
====/plugins/index.php====
====/plugins/index.php====
* Would be nice to move this information into a database.
====/plugins/download.php====
* [61-78] PHP is not for serving files.  Is there a better way to get the user to their file?
====/plugins/PluginFinderService.php====
* [57][92] Functions that are declared but never used.
* [50] The infamous bail() function has no purpose.
* [42-45] DB information is stored in this file.  Why???
* [47] This should be a require_once().
* [394] This could be done more thoroughly with a built-in PHP function instead of using preg_replace.
====/plugins/uniqueUrl.php====
* Can't touch this.  Dun dun dun dun dun dun dun dun.
----
----
===/rss===
===/rss===
====/rss/inc_rssfeed.php====
====/rss/inc_rssfeed.php====
Line 521: Line 591:
===/update===
===/update===
====/update/VersionCheck.php====
====/update/VersionCheck.php====
* Versions should not be mathematically illogical.  i.e. 2.10 should not be a higher version than 2.2! ... :| (alanjstr says: it is in the Gnu and Mozilla World.  Take a look at the bugzilla product)
* [42] DB connection should not happen until after all inputs have been determined and validated
* [42] DB connection should not happen until after all inputs have been determined and validated
* [49] Header sent before contents are determined
* [49] Header sent before contents are determined
* [52] This is a pointless wrapper.
* [52] This is a pointless wrapper.
* [63,113,124,138,144,265] bail() is questionable; shouldn't we let things die quietly?
* [63,113,124,138,144,265] bail() is questionable; shouldn't we let things die quietly? (alanjstr says: can we make bail() output valid XML, maybe outputting the correct format of input?)
* [59+] Functions defined during script - this should be separated; the logic behind the script is hard to determine because function definitions interrupt it
* [59+] Functions defined during script - this should be separated; the logic behind the script is hard to determine because function definitions interrupt it
* [127-132] This seemingly arbitrary variable renaming makes it hard to follow where data originates.
* [127-132] This seemingly arbitrary variable renaming makes it hard to follow where data originates.
Line 531: Line 602:
** Check to see if HTTP_USER_AGENT is empty or not before doing any checks on it
** Check to see if HTTP_USER_AGENT is empty or not before doing any checks on it
** > 0 is not sufficient; it should be !== FALSE since strpos can return 0 as a valid return
** > 0 is not sufficient; it should be !== FALSE since strpos can return 0 as a valid return
** Code comments indicate 1-6, checks provide IDs 0,2,3,4,5,6
** Code comments indicate 1-6, checks provide IDs 0,2,3,4,5,6 (alanjstr says: replace this with names, not ints)
** Use logic to create short-circuit; right now it's all if's so the checks will happen even after a match is found and $osid is set -- logically, it's possible for $osid to be set twice
** Use logic to create short-circuit; right now it's all if's so the checks will happen even after a match is found and $osid is set -- logically, it's possible for $osid to be set twice
* [234] Query in a loop.  Remove this.  This could be replaced with an IN( explode(','$reqItemGuid) ) instead of $reqItemGuid[$i] in the query (assuming some checks on $reqItemGuid before the explode()), reducing the number of queries from count($reqItemGuid) to 1 since $osid and $reqTargetAppGuid are the same.
* [234] Query in a loop.  Remove this.  This could be replaced with an IN( implode(','$reqItemGuid) ) instead of $reqItemGuid[$i] in the query (assuming some checks on $reqItemGuid before the implode()), reducing the number of queries from count($reqItemGuid) to 1 since $osid and $reqTargetAppGuid are the same. (alanjstr says: good catch.  This is new code, so it only handles 1 at a time right now)
* [253] Is there any way to avoid this OR ?
* [253] Is there any way to avoid this OR ?
* The mime-type "text/rdf" should be "text/xml".
Fligtar
canmove, Confirmed users, Bureaucrats and Sysops emeriti
1,043

edits

Retrieved from "https://wiki.allizom.org/Special:MobileDiff/3305...55466"