4892: Fix all PHP errors and warnings currently suppressed by error_reporting(0) #11
Conversation
variables $keywords, $caller_script indexes 'user_id', 'user_role'
* Use maximum error reporting to ensure focus on problems * Use the correct setcookie() function variables $_custom_head indexes 'user_id'
* declare global for $starttime to ensure it is in-scope when footer.tmpl.php
needs it to calculate page-rendering elapsed time
* correct $base_href to $base_path in Javascript include for ProgressiveEnhancement.js
indexes 'cid'
properties onload, sequence_links, sub_level_pages
* declare global for $starttime to ensure it is in-scope when it is
used to calculate page-rendering elapsed time
index 'user_requirement'
* tidy up code to append QueryString to URI variables $url_param
index 'user_requirement'
indexes $parent_item_id, $current_item_id
* fix Accessing static property DAO::$db as non static
variables $common_path, $contains_glossary_terms, $package_base_name, $package_base_name_url, $glossary_path
variables $_content_id
index $content['parent_content_id']
* ensure returns by-value rather than by-reference * ensure array indexes are defined before using them
…msg->containsErrors() This function would erroneously return 'false' if the $msg array contained errors prior to entry into the function. This sometimes caused unexplained IMS CC import failures when inserting new lessons into the database. Use a local return result to ensure accurate result.
…es not use $last_element
…this->sequence_links
* Weblinks: correctly locate link resource file by replacing incorrect
$content_info['href'] with $content_info['file'][0]
* Weblinks: ensure weblink file has been parsed correctly before assigning variables
* capture and use result from $contentDAO->Create()
* undefined indexes: $items[$current_item_id], $items[$dependency_ref],
$items[$content_info['parent_content_id']], $items[$item_id]['type']
$content_info['type'], $content_info['dependency'], $content_info['href'], $content_info['new_path'],
* undefined variables: $ext, $full_filename, $lti_offset[$content_info['parent_content_id']],
* undefined variables: $_content_id, $_sequence_links
…te author of a course
| $new_item['ordering'] = $parent_obj['ordering']; | ||
| if ($new_item['parent_content_id']!='0'){ | ||
| $new_item['ordering']++; | ||
| if (isset($content['parent_content_id'])) { |
There was a problem hiding this comment.
Just a suggestion that you may want to consider using:
if (!isset($content['parent_content_id'])) continue;
which would reduce one layer of if () {...} indentation and make the code easier to read.
There was a problem hiding this comment.
I'm approaching it in two phases:
- Fix all existing Warnings at their point of origin
- Re-factor code so that all variables are declared/defined before first-use
- Remove redundant conditional existence tests
(2) will be done later and will lead naturally to (3), which I hope will result in the code being much cleaner to read, use, and develop.
There was a problem hiding this comment.
Thanks for the clarification and the great planning. It's might be more worthwhile to merge in the code after all the 3 steps are done rather than reviewing and testing the same code over and over again. What do you think?
There was a problem hiding this comment.
I'd prefer to do it incrementally starting with these immediate fix-ups and then refactor the code at leisure - which may take several months overall (including looking at ATutor). Each of these Warnings causes at least 2 unwanted side effects.
Most importantly, some of them indicate a failure of the code to do what the programmer expected and may be the cause of some conditional branches effectively being dead code since they'll never be executed.
Secondly, the PHP JIT compiler is following the language-exception path (which by definition is the slow path) meaning that the execution of the code on each page-load is taking more CPU resource and more time than would be the case if the code is fixed.
On busy servers this cumulative time expended by the CPU leads to a slower response to all requests and more CPU resource-per-request consumed.
There was a problem hiding this comment.
I didn't expect that some code in the conditional branch could be dead due to the PHP warning or notice. If it does, what a shame. Yes, we should use the highest error reporting level at development which was ignored. Thanks for catching those and reporting to us.
Back to this piece of code, I do understand your step-by-step code refactoring, which is generally applied to the whole system. However, my debate is, since we are working on this piece of code and we know that there's room for it to be polished right away, why do we want to leave it to the re-visit?
There was a problem hiding this comment.
Well, from my perspective, I'm doing all this simply to fix problems as I learn to use ATutor/ACotent for an actual project. Fixing the code-base so it works as intended/expected is my first objective so that I can get on with providing an education to my student! That also gives me the opportunity to learn the entire code-base and how the components interact. That'll give me an overview that will help me to later re-factor the code in a well-planned and designed manner - I need to understand it instinctively to do that.
If you have the time and resources available to do it now then by all means go to it! My own time demands mean it'll be an ongoing process over the next 6 months or so for me - I hope to use it to actually teach my BSc Computer Science student about real code maintenance and the complexities of evolving code, and have him get involved in the coding.
see http://atutor.ca/atutor/mantis/view.php?id=4892