canmove, Confirmed users, Bureaucrats and Sysops emeriti
1,043
edits
Update:Archive/1.0/Developers: Difference between revisions
no edit summary
No edit summary |
|||
| (29 intermediate revisions by 3 users not shown) | |||
| Line 1: | Line 1: | ||
{{AmoArchive}} | |||
== Introduction == | == Introduction == | ||
| Line 233: | Line 233: | ||
** <strike>Efficiency issues that really need to be changed</strike> | ** <strike>Efficiency issues that really need to be changed</strike> | ||
** <strike>Efficiency or style issues that are cosmetic</strike> | ** <strike>Efficiency or style issues that are cosmetic</strike> | ||
* Test behavior in identical staging environment for all public use-cases. ( | * Test behavior in identical staging environment for all public use-cases. (morgamic, chenb) | ||
=== Code Adjustments === | === Code Adjustments === | ||
* Patch branch to include local modifications in prod. (under review) | * <strike>[https://bugzilla.mozilla.org/show_bug.cgi?id=283215 Patch branch to include local modifications in prod.] (under review)</strike> | ||
* Update | * Patching | ||
** | ** <strike>Rewrite VersionCheck.php</strike> (morgamic) | ||
** | *** <strike>This is currently being reviewed. (mconnor)</strike> | ||
** Deal out important patching to community based on wiki information. (morgamic,alanjstr) | ** <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 === | ||
| Line 273: | 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 307: | Line 329: | ||
* 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. | * 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==== | ||
| Line 313: | Line 336: | ||
* 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. | * 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. | * 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 318: | 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 418: | 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 472: | 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 491: | Line 518: | ||
===/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 511: | 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 531: | 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 | ||
| Line 545: | Line 606: | ||
* [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) | * [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". | |||