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

Fixes database queries related to loading content libraries #4

Merged
merged 1 commit into from
Jan 4, 2016
Merged

Fixes database queries related to loading content libraries #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
74 changes: 50 additions & 24 deletions hvp.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,9 +539,9 @@ public function loadLibrarySemantics($name, $majorVersion, $minorVersion) {
$semantics = $DB->get_field_sql(
"SELECT semantics
FROM {hvp_libraries}
WHERE name = %s
AND major_version = %d
AND minor_version = %d",
WHERE machine_name = ?
AND major_version = ?
AND minor_version = ?",
array($name, $majorVersion, $minorVersion)
);

Expand All @@ -561,26 +561,43 @@ public function alterLibrarySemantics(&$semantics, $name, $majorVersion, $minorV
public function loadContent($id) {
global $DB;

// TODO: Verify that this work with other databases than MySQL. I suspect
// that camelCase won't work for e.g. PostgreSQL
$content = $DB->get_record_sql(
$data = $DB->get_record_sql(
"SELECT hc.id
, hc.json_content AS params
, hc.name
, hc.json_content
, hc.filtered
, hc.slug
, hc.embed_type AS embedType
, hc.embed_type
, hc.disable
, hl.id AS libraryId
, hl.machine_name AS libraryName
, hl.major_version AS libraryMajorVersion
, hl.minor_version AS libraryMinorVersion
, hl.embed_types AS libraryEmbedTypes
, hl.fullscreen AS libraryFullscreen
, hl.id AS library_id
, hl.machine_name
, hl.major_version
, hl.minor_version
, hl.embed_types
, hl.fullscreen
FROM {hvp} hc
JOIN {hvp_libraries} hl ON hl.id = hc.main_library_id
WHERE hc.id = ?", array($id)
);

// Some databases do not support camelCase, so we need to manually
// map the values to the camelCase names used by the H5P core.
$content = array(
'id' => $data->id,
'title' => $data->name,
'params' => $data->json_content,
'filtered' => $data->filtered,
'slug' => $data->slug,
'embedType' => $data->embed_type,
'disable' => $data->disable,
'libraryId' => $data->library_id,
'libraryName' => $data->machine_name,
'libraryMajorVersion' => $data->major_version,
'libraryMinorVersion' => $data->minor_version,
'libraryEmbedTypes' => $data->embed_types,
'libraryFullscreen' => $data->fullscreen,
Copy link
Author

Choose a reason for hiding this comment

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

Custom array key names are added here manually just so that they can be immediately renamed again into another array by H5PCore::loadContent()in https://github.com/h5p/h5p-php-library/blob/master/h5p.classes.php#L1735

I'm noticing a bit of a code smell... :)

I think in the long-term the integration library should be refactored to always pass objects as function arguments. That way properties could always be accessed with methods instead of arbitrary array keys. Additionally input type hinting could be used to further clarify what each method is expecting.

I opened a separate issue for further planning and discussion: h5p/h5p-php-library#11

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is far from ideal.
I suggest we get it working before we start any refactoring.

);

return $content;
}

Expand All @@ -592,14 +609,14 @@ public function loadContentDependencies($id, $type = NULL) {

$query =
"SELECT hl.id
, hl.machine_name AS machineName
, hl.major_version AS majorVersion
, hl.minor_version AS minorVersion
, hl.patch_version AS patchVersion
, hl.preloaded_css AS preloadedCss
, hl.preloaded_js AS preloadedJs
, hcl.drop_css AS dropCss
, hcl.dependency_type AS dependencyType
, hl.machine_name
, hl.major_version
, hl.minor_version
, hl.patch_version
, hl.preloaded_css
, hl.preloaded_js
, hcl.drop_css
, hcl.dependency_type
FROM {hvp_contents_libraries} hcl
JOIN {hvp_libraries} hl ON hcl.library_id = hl.id
WHERE hcl.hvp_id = ?";
Expand All @@ -611,7 +628,14 @@ public function loadContentDependencies($id, $type = NULL) {
}

$query .= " ORDER BY hcl.weight";
return $DB->get_records_sql($query, $queryArgs);
$data = $DB->get_records_sql($query, $queryArgs);

$dependencies = array();
foreach ($data as $dependency) {
$dependencies[] = H5PCore::snakeToCamel($dependency);
}

return $dependencies;
}

/**
Expand Down Expand Up @@ -639,6 +663,8 @@ public function updateContentFields($id, $fields) {
global $DB;

$content = new stdClass();
$content->id = $id;

foreach ($fields as $name => $value) {
$content->$name = $value;
// TODO: Verify that this works, we might have to convert $name from
Expand Down Expand Up @@ -700,7 +726,7 @@ public function saveLibraryUsage($contentId, $librariesInUse) {

foreach ($librariesInUse as $dependency) {
$dropCss = in_array($dependency['library']['machineName'], $dropLibraryCssList) ? 1 : 0;
$DB->insert_record('h5p_contents_libraries', array(
$DB->insert_record('hvp_contents_libraries', array(
'hvp_id' => $contentId,
'library_id' => $dependency['library']['libraryId'],
'dependency_type' => $dependency['type'],
Expand Down
5 changes: 3 additions & 2 deletions view.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,7 @@
$hvp->disable |= $core->getGlobalDisable();
}

// TODO: Create Content!
throw new coding_exception('Create proper $content before filtering parameters');
$content = $core->loadContent($cm->instance);

// Filter content parameters
$safe_parameters = $core->filterParameters($content);
Expand All @@ -75,6 +74,8 @@

// Get assets for this content
$preloaded_dependencies = $core->loadContentDependencies($hvp->id, 'preloaded');
$preloaded_dependencies = $core->loadContentDependencies($cm->instance, 'preloaded');

$files = $core->getDependenciesFiles($preloaded_dependencies);
// TODO:Insert hook/event for altering assets?

Expand Down