Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

4888: Undefined Index warnings during IMS CC import #8

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

4888: Undefined Index warnings during IMS CC import #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
eb0e501
4888: ims_import.php: prevent "Undefined index: -1" warning
Oct 10, 2011
2bb149b
4888: ims_import.php: prevent "Undefined index: identifierref and par…
Oct 10, 2011
dabd698
4888: ims_import.php: prevent "Undefined index: href in startElement()
Oct 10, 2011
64a82d8
4888: ims_import.php: prevent "Undefined index: -1 in endElement()
Oct 10, 2011
bde9900
4888: ims_import.php: prevent "Undefined index: type in checkResources()
Oct 10, 2011
7dc9105
4888: prog.php: prevent "Undefined index: tile and t" and "Undefined …
Oct 10, 2011
7076b3a
4888: prog.php: prevent "Undefined index:" frame, done and t
Oct 11, 2011
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 17 additions & 15 deletions home/ims/ims_import.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ function checkResources($import_path){
}

//validate the xml by its schema
if (preg_match('/imsqti\_(.*)/', $fileinfo['type'])){
if (isset($fileinfo['type']) && preg_match('/imsqti\_(.*)/', $fileinfo['type'])){
$qti = new QTIParser($fileinfo['type']);
$xml_content = @file_get_contents($import_path . $fileinfo['href']);
$qti->parse($xml_content); //will add error to $msg if failed
Expand Down Expand Up @@ -455,7 +455,7 @@ function startElement($parser, $name, $attrs) {
global $course_primary_lang;

// get language from CONTENT PACKAGE
if (substr($element_path[count($element_path)-1], -6) == ':title' && substr($name, -11) == ':langstring') {
if (count($element_path) > 0 && substr($element_path[count($element_path)-1], -6) == ':title' && substr($name, -11) == ':langstring') {
$course_primary_lang = trim($attrs['xml:lang']);
}

Expand Down Expand Up @@ -498,7 +498,7 @@ function startElement($parser, $name, $attrs) {
// special case for webCT content packages that don't specify the `href` attribute
// with the `<resource>` element.
// we take the `href` from the first `<file>` element.
if (isset($items[$current_identifier]) && ($items[$current_identifier]['href'] == '')) {
if (isset($items[$current_identifier]) && isset($items[$current_identifier]['href']) && ($items[$current_identifier]['href'] == '')) {
$attrs['href'] = urldecode($attrs['href']);
$items[$current_identifier]['href'] = $attrs['href'];
}
Expand Down Expand Up @@ -565,15 +565,15 @@ function startElement($parser, $name, $attrs) {
if (isset($_POST['allow_a4a_import']) && isset($items[$current_identifier])) {
$items[$current_identifier]['a4a_import_enabled'] = true;
}
} else if (($name == 'item') && ($attrs['identifierref'] != '')) {
} else if (($name == 'item') && array_key_exists('identifierref', $attrs) && ($attrs['identifierref'] != '')) {
$path[] = $attrs['identifierref'];
} else if (($name == 'item') && ($attrs['identifier'])) {
$path[] = $attrs['identifier'];

} else if (($name == 'resource')) {
$current_identifier = $attrs['identifier'];
$items[$current_identifier]['type'] = $attrs['type'];
if ($attrs['href']) {
if (isset($attrs['href'])) {
$attrs['href'] = urldecode($attrs['href']);

$items[$attrs['identifier']]['href'] = $attrs['href'];
Expand All @@ -596,7 +596,7 @@ function startElement($parser, $name, $attrs) {
//if there is a dependency, attach it to the item array['file']
$items[$current_identifier]['dependency'][] = $attrs['identifierref'];
}
if (($name == 'item') && ($attrs['parameters'] != '')) {
if (($name == 'item') && array_key_exists('parameters', $attrs) && ($attrs['parameters'] != '')) {
$items[$attrs['identifierref']]['test_message'] = $attrs['parameters'];
}
if ($name=='file'){
Expand Down Expand Up @@ -633,17 +633,19 @@ function endElement($parser, $name) {

// added by Cindy Li on Jan 10, 2010
// Extract course title, description and primary language for a newly-created course
if (substr($element_path[count($element_path)-2], -6) == ':title') {
if (substr($element_path[count($element_path)-1], -7) == ':string' ||
substr($element_path[count($element_path)-1], -11) == ':langstring') {
$course_title = trim($my_data);
if (count($element_path) >= 2) {
if (substr($element_path[count($element_path)-2], -6) == ':title') {
if (substr($element_path[count($element_path)-1], -7) == ':string' ||
substr($element_path[count($element_path)-1], -11) == ':langstring') {
$course_title = trim($my_data);
}
}
}

if (substr($element_path[count($element_path)-2], -12) == ':description') {
if (substr($element_path[count($element_path)-1], -7) == ':string' ||
substr($element_path[count($element_path)-1], -11) == ':langstring') {
$course_description = trim($my_data);
if (substr($element_path[count($element_path)-2], -12) == ':description') {
if (substr($element_path[count($element_path)-1], -7) == ':string' ||
substr($element_path[count($element_path)-1], -11) == ':langstring') {
$course_description = trim($my_data);
}
}
}

Expand Down
46 changes: 24 additions & 22 deletions home/prog.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
define('TR_INCLUDE_PATH', '../include/');
require(TR_INCLUDE_PATH.'vitals.inc.php');
session_write_close();
if ($_GET['tile']) {
if (isset($_GET['tile']) && $_GET['tile']) {
$lang_variable = 'tile_progress';
} else {
$lang_variable = 'upload_progress';
Expand All @@ -26,19 +26,19 @@
<html lang="<?php echo $myLang->getCode(); ?>">
<head>
<title><?php echo _AT($lang_variable); ?></title>
<?php if ($_GET['frame']) { ?>
<?php if (array_key_exists('frame', $_GET)) { ?>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 18 uses "isset($_GET['tile']) && $_GET['tile']" to check the presence of the $_GET value while this line uses another approach. Should we make it consistent by using one way through? The same comment applies to line 36, 41.

BTW, I personally likes the line 18 way better. :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference is that isset() will return false for array keys that have their value set to NULL whereas array_key_exists() only checks that the key exists.

That said, I'm struggling to recall the reason I used these styles of code. I think it was because I believed or saw that some of the GET variables may be NULL e.g. "/home/prog.php" rather than "/home/prog.php?done"

<META HTTP-EQUIV="refresh" content="3;URL=prog.php?frame=1">
<?php } ?>
<link rel="stylesheet" href="<?php echo $_base_path; ?>themes/<?php echo $_SESSION['prefs']['PREF_THEME']; ?>/styles.css" type="text/css" />
<meta http-equiv="Content-Type" content="text/html; <?php echo $myLang->getCharacterSet(); ?>" />
</head>
<body <?php
if ($_SESSION['done']) {
if (array_key_exists('done', $_SESSION)) {
echo 'onload="parent.window.close();"';
}
?>>
<?php
if (!$_GET['frame']) { ?>
if (!array_key_exists('frame', $_GET)) { ?>
&nbsp;<a href="javascript:window.close();"><?php echo _AT('close'); ?></a>
<h3><?php echo _AT($lang_variable); ?></h3>
<p><small><?php echo _AT('window_auto_close'); ?></small></p>
Expand All @@ -51,30 +51,32 @@
</iframe>
<?php } else {
$tmp_dir = ini_get('upload_tmp_dir') . DIRECTORY_SEPARATOR;
if (!$_GET['t']) {
$newest_file_name = '';
$newest_file_time = 0;
// get the name of the temp file.
if ($dir = @opendir($tmp_dir)) {
while (($file = readdir($dir)) !== false) {
if ((strlen($file) == 9) && (substr($file, 0, 3) == 'php')) {
$filedata = stat($tmp_dir . $file);
if ($filedata['mtime'] > $newest_file_time) {
$newest_file_time = $filedata['mtime'];
$newest_file_name = $file;
$size = $filedata['size'] / 1024;
if (isset($_GET['t'])) {
if (!$_GET['t']) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 2 lines above are the translation of the original code of "if (!$_GET['t'])". The translation seems missing the condition of (!isset($_GET['t']))?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where the relaxed style of PHP can cause logical failures. As far as I can tell, the intent of the code "if (!$_GET['t'])" is to be TRUE when there is an empty 't' key in $_GET. However, the "!$_GET['t']" logical condition evaluates to TRUE both for "/home/prog.php" and "/home/prog.php?t". Therefore this code branch was being executed unexpectedly when 't' wasn't in $_GET[].

I'm not even sure this code should be in AContent at all - it seems to be a left-over of a copy-paste from ATutor's tools/prog.php to support tile progress.

The replacement uses an isset() wrapper around the original code to ensure 't' is set and then checks (!$_GET['t']) it doesn't provide a value (which would be handled in the associated else {} clause) and indents the contained code to help the programmers eye follow it.

$newest_file_name = '';
$newest_file_time = 0;
// get the name of the temp file.
if ($dir = @opendir($tmp_dir)) {
while (($file = readdir($dir)) !== false) {
if ((strlen($file) == 9) && (substr($file, 0, 3) == 'php')) {
$filedata = stat($tmp_dir . $file);
if ($filedata['mtime'] > $newest_file_time) {
$newest_file_time = $filedata['mtime'];
$newest_file_name = $file;
$size = $filedata['size'] / 1024;
}
}
}
closedir($dir);
}
closedir($dir);
} else {
$filedata = stat($tmp_dir . $_GET['t']);
$size = $filedata['size'] / TR_KBYTE_SIZE;
}
} else {
$filedata = stat($tmp_dir . $_GET['t']);
$size = $filedata['size'] / TR_KBYTE_SIZE;
}
// not sure where these are displayed in the progress popup
// displayed in the small iframe alongside the progress animated image
echo '<small>';
if ($size == '') {
if (!isset($size) || isset($size) && $size == '') {
echo '<em>'._AT('unknown').' </em> '._AT('kb');
} else {
echo number_format($size, 2).' '._AT('kb');
Expand Down