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.

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

Add Custom Endpoints for H5P activities using the WP REST API #63

Closed
SteelWagstaff opened this issue Nov 30, 2017 · 34 comments
Closed

Add Custom Endpoints for H5P activities using the WP REST API #63

SteelWagstaff opened this issue Nov 30, 2017 · 34 comments

Comments

@SteelWagstaff
Copy link

SteelWagstaff commented Nov 30, 2017

I'm working at a university in North America and supporting a number of teachers who are working to publish open textbooks. For many of our authors, we authoring our texts using Pressbooks and then embedding interactive content using the H5P plugin for WordPress. Pressbooks also has an awesome API for books that's built on the WP REST API that lets us do some really amazing things with cloning books from any open network to any other open network. One problem that we’re encountering is that we haven’t yet tightly integrated H5P activities with the Pressbooks cloning routine. My understanding is that while H5P does provide an javascript API: https://h5p.org/documentation/api/H5P.html, it doesn’t currently produce the REST API endpoints we would need to clone these activities along with other content in a Pressbooks.

As I understand it, in order to allow H5P activities in book to be cloned using the Pressbooks cloning tool, two things need to happen: 1) REST API endpoints need to be added to H5P activities via the WordPress REST API and 2) The Pressbooks cloning routine should be modified so that it looks for and copy/clones any H5P activities exposed via the WordPress REST API. We're interested in working with the H5P developer community to do step 1, namely, to add custom endpoints to H5P activities in this WordPress plugin, and would consider funding this development or helping to contribute to a pull request to that end. Thoughts/advice/discussion/direction welcome!

@timothyylim
Copy link
Contributor

Hi @SteelWagstaff, I've assigned this to @icc. He's on holiday right now but he'll get back to you in due time!

@icc
Copy link
Member

icc commented Dec 19, 2017

This sounds like an interesting project and without looking at what's really needed in details, I can't see any reason why the H5P plugin shouldn't offer these kinds of endpoints and features. I'm happy to help or contribute in any way I can.
If you'd like an offer on the work, feel free to send us a detail description of the requirements.

@dac514
Copy link

dac514 commented Feb 7, 2018

Hi, i'd like to chime in for our needs at Pressbooks.

Our OER users want to move openly licenced H5P content from one server to another. In Pressbooks we call this "Cloning". Cloning happens using the REST API. That said this could also be done using WXR (The WordPress eXtended Rss XML document) which is a generic WordPress feature that you should also consider if you are coding this.

In both import/export cases mentioned above, all we get in the data is something like [h5p id="999"].

Importing that shortcode into a target system, as you can hopefully see, doesn't help much. There is no 999 on the target system, and we can't infer anything from the 999 outside of the source system (we don't have access to the source database).

If we copy the embed iframe, then the interactivity feeds back into the old system (or is simply broken). This is undesired when cloning.

IDEAS:

The h5p-wordpress-plugin allows user to download H5P files (I.e. I can download using the Download button next to the Embed button when viewing H5P content in a webpage). Its a ZIP file with JSON and JS in it. The URL is something along the lines of a link to : /uploads/sites/XXX/h5p/exports/my-multiple-choice-test-1.h5p On most WordPress installs I can guess this URL. An ugly hack. I would like your REST API enpoint(s) to show me the path to this file.

Next, we would like a PHP function in h5p-wordpress-plugin that works something like wp_handle_sideload. Right now, we would have to somehow fake a $_POST to get that external H5P file back into the system. Again, a yucky hack. This new function should allow us to import the H5P file, it should then return an ID that we can use to swap [h5p id="999"] with.

Here are some WordPress docs on how a plugin can add WordPress endpoints:

Thank you for your consideration.

@SteelWagstaff
Copy link
Author

Hi @icc — hopefully @connerbw’s description suffices for the technical requirements scope needed for this request. If you would need ‘sponsorship’ for this development request, please feel free to send a cost estimate to me at swagstaff@wisc.edu. Adding these endpoints (and allowing for centralized library management and/or bulk updating of H5P libraries) are a very high priority for us.

@icc
Copy link
Member

icc commented Feb 8, 2018

@connerbw Thanks for the clarification. It doesn't seem like this requires too much work on the H5P plugin's part. Though, I've got a couple of questions for the two different parts needed:

  1. When a post or another piece of content is requested is there a process that goes through the content(on the receiver) and "looks up" shortcodes so that the H5P data(title, tags, .h5p url) can be embedded into the same response? If so, what is the filter or hook that needs implementing called?
    Or does the receiver have to look through the content and find the IDs for the content and then make another request? I.e. the H5P plugin will have to create an API endpoint that will receive these requests?

  2. What will trigger the H5P plugin to download the file and create the content? Is it some sort of generic hook or will you be making a specific call to e.g. $newid = H5P_Plugin::create_from($url);

If you prefer to point to any code or examples that is fine.

@dac514
Copy link

dac514 commented Feb 8, 2018

1

I'm imagining the h5p-wordpress-plugin provides an endpoint like how Pressbooks provides one for section metadata:

https://book.pressbooks.com/wp-json/pressbooks/v2/chapters/4/metadata

When splitting the above URL "chapters" can be thought of as a post, the number 4 is the post ID and metadata is the endpoint done using a custom controller.

Your endpoint could be something like:

https://localhost/site/wp-json/h5p/v1/post/4

In it you could have JSON that looks like:

[
  {
    "id": 999,
    "url": "https://localhost/site/XXX/h5p/exports/my-multiple-choice-test-1.h5p"
  },
  {
    "id": 111,
    "url": "https://localhost/site/XXX/h5p/exports/there-are-2-interactive-elements-in-post-4.h5p"
  }
]

This would be enough for us. The more data the better! (titles, licenses, etc)

2

All we need is $newid = H5P_Plugin::create_from($file) Where $url is instead $file. If we have enough info in the API then we can handle downloading and slap chopping the data on our end . If you want to implement downloading URLs that's a bonus.

Hope this helps.

@icc
Copy link
Member

icc commented Feb 9, 2018

  1. Sounds good to me. I assume this means that the H5P plugin would have to load the post body and filter out all the [h5p] shortcodes. Will we be supporting anything else like pages or comments?

  2. OK, good.

@dac514
Copy link

dac514 commented Feb 9, 2018

If you build your own WP REST API controller, and can recover post_id from the URL, then something like:

$post = get_post( $id );
$data = array();
if ( preg_match_all( '/' . get_shortcode_regex() . '/s', $post->post_content, $matches, PREG_SET_ORDER ) ) {
	foreach ( $matches as $shortcode ) {
		if ( 'h5p' === $shortcode[2] ) {
			$attrs = shortcode_parse_atts( $shortcode[3] );
			$id = $attrs['id']
			// You now have an ID
			// Look in the database, do stuff.
			// Append to $data
		}
	}	
}
return return rest_ensure_response( $data ); // JSON

(Untested... Hope this helps?)

@icc
Copy link
Member

icc commented Feb 9, 2018

  1. Thanks for the example. I'll assume that this means posts only then.

@dac514
Copy link

dac514 commented Feb 9, 2018

Oh right, yes. We only care about posts. However, our posts are custom. You can recover them with the same ID as you would a regular post but $post->post_type will be front-matter, back-matter, part, chapter, ...

If this doesn't work out of the box then filters would be appreciated.

@icc
Copy link
Member

icc commented Feb 9, 2018

@connerbw Is there any specific access check needed, or can anyone get this? Probably only if the current user can edit, or are there some API keys being used?
If not does it really matter if the request goes through a controller or if it's just a regular admin-ajax.php?action=h5p_in_post&id=N?

@dac514
Copy link

dac514 commented Feb 9, 2018

Our permission methods look like this:

	public function get_item_permissions_check( $request ) {
		if ( current_user_can( 'edit_posts' ) ) {
			return true;
		}
		if ( get_option( 'blog_public' ) ) {
			return true;
		}
		return false;
	}

We use get_option( 'blog_public' ) to mean "Open Book". Not standard in WP ecosystem. Filter appreciated.

@dac514
Copy link

dac514 commented Feb 9, 2018

API keys being used

WP API respects permissions but the developer must setup authentication separately. [1]

https://developer.wordpress.org/rest-api/using-the-rest-api/authentication/#authentication-plugins

@SteelWagstaff
Copy link
Author

For anyone following this issue, UW-Madison has funded this development and our hope that it will be delivered within the next two months (by end of October).

icc added a commit that referenced this issue Nov 16, 2018
Also, includes new function that makes it easy to fetch
and store remote content.

JI-907
@icc
Copy link
Member

icc commented Nov 16, 2018

I've added the requested features in 1b27e5f.
Could you guys test it and give me any feedback on how it works for your system?

The new function for downloading and storing .h5p files works like this:

$plugin = H5P_Plugin::get_instance();
$h5p_id = $plugin->fetch_h5p('http://localhost/wp-content/uploads/h5p/exports/troms-10.h5p');

Have a nice weekend!

@dac514
Copy link

dac514 commented Nov 16, 2018

Since I last wrote, we've done work around attachments. One of the things we took advantage of was WordPress lists all media attachments at a single endpoint. We don't download all the attachments. We check if we can find it in a post, but a lot of time is saved by getting all the info in a single request in a "setup" step.

Can you consider adding /wp-json/h5p/v1/all (or similar)?

    register_rest_route('h5p/v1', 'all', array(
            'methods' => 'GET',
            'callback' => array($this, 'rest_api_all'),
            'permission_callback' => function () {
                return apply_filters('h5p_rest_api_all_permission', current_user_can('edit_others_h5p_contents'));
            }
        ) );
public function rest_api_all(WP_REST_Request $request) {
        global $wpdb;
        $results = $wpdb->get_results( "SELECT hc.id,hc.slug FROM {$wpdb->prefix}h5p_contents hc");
        $data = array();
        foreach ($results as $h5p) {
            $data[] = (object) array(
                'id' => $h5p->id,
                'url' => $this->get_h5p_url(true) . '/exports/' . ($h5p->slug ? $h5p->slug . '-' : '') . $h5p->id . '.h5p'
            );
        }
        return $data;
    }

Code is copy/pasta, the foreach ($results as $h5p) is a good candidate for a "don't repeat yourself" function that can print the same data for a single post, for all, etc.

Thanks.

@dac514
Copy link

dac514 commented Nov 16, 2018

Note that WordPress paginates the /wp/v2/media endpoint. We don't need pagination. Just pointing out that there exists a convention.

Source

icc added a commit that referenced this issue Nov 21, 2018
@icc
Copy link
Member

icc commented Nov 21, 2018

Ok, I've added it without any limit or pagination.

@dac514
Copy link

dac514 commented Nov 21, 2018

Wordpress has a feature where it can use the REST API internally.

$_GET['_embed'] = 1;
$request = new \WP_REST_Request( 'GET', "/h5p/v1/all" );
$response = rest_do_request( $request );

When called like that, this code crashes PHP (the crash happens in \WP_REST_Server::embed_links)

  $data[] = (object) array(
	'id' => $h5p->id,
	'url' => "{$baseurl}/exports/{$slug}{$h5p->id}.h5p"
  );

The crash happens because PHP is expecting an array when instead it gets an object. Please remove the (object) casting.

  $data[] = array(
	'id' => $h5p->id,
	'url' => "{$baseurl}/exports/{$slug}{$h5p->id}.h5p"
  );

And wrap the returns in rest_ensure_response

return rest_ensure_response($this->get_h5p_exports_list($ids) );
return rest_ensure_response( $this->get_h5p_exports_list() );

@dac514
Copy link

dac514 commented Nov 21, 2018

WordPress Multisite Example:

 + Blog 1 - H5P
 + Blog 2 
 + Blog 3 - H5P
 + Blog 4
 + Blog 5

Five blogs, but the H5P plugin is only enabled in Blog 1 and Blog 3.

When I run $p->fetch_h5p( $url ); on Blog 4 it does a lot of work for nothing.

  • Expected: Throw an exception much earlier in the process, ideally before even attempting to download anything.
  • Actual: Success with new id of 0?

@dac514
Copy link

dac514 commented Nov 22, 2018

WordPress multisite problem.

In a Pressbooks cloning operation, we create a new book.

When we move content from Old Book 1 to New Book 2 and use $plugin->fetch_h5p( $url ) command (with some success at this point in time)

We get hundreds of:

Warning: file_get_contents(/srv/www/pressbooks.test/current/web/app/uploads/sites/154/h5p/libraries/H5P.TrueFalse-1.5/styles/h5p-true-false.css): failed to open stream: No such file or directory in /srv/www/pressbooks.test/current/web/app/plugins/h5p/h5p-php-library/h5p-default-storage.class.php on line 203

These files don't come along for the ride.

@dac514
Copy link

dac514 commented Nov 22, 2018

I think the problem is when get_h5p_instance sets the path to, for example:

H5PDefaultStorage Object
(
    [path:H5PDefaultStorage:private] => /srv/www/pressbooks.test/current/web/app/uploads/sites/140/h5p
    [alteditorpath:H5PDefaultStorage:private] => 
)

Somewhere before a switch_to_blog(154) and then fetch_h5p.

H5PDefaultStorage has locked the path to 140 because, singletons...

icc added a commit that referenced this issue Nov 22, 2018
@icc
Copy link
Member

icc commented Nov 22, 2018

Added a fix for the REST API things you mentioned.

Yes, the path is set the first time getUploadedH5pPath() is called and will remain the same throughout the request. Maybe adding an optional variable to reset or set a custom path will do the trick work?

@dac514
Copy link

dac514 commented Nov 22, 2018

Maybe adding an optional variable to reset or set a custom path will do the trick work?

Just a heads up, I fudged in:

public function fetch_h5p($url) {
  // Override core permission checks
  self::$interface = null;
  $core = $this->get_h5p_instance('core');

Just to prove that this could at least work, but I had trouble with the underlying function:

self::$core = new H5PCore(self::$interface, $this->get_h5p_path(), $this->get_h5p_url(), $language, get_option('h5p_export', TRUE));

More specifically:

\H5P_Plugin::get_h5p_url

Has a static $url; in it. I could not find a trivial way to reset it.

Maybe be worth investigating, something like:

// \H5P_Plugin::get_h5p_instance

/**
 * @var \H5PWordPress[]
 */
protected static $interface = [];

/**
 * @var \H5PCore[]
 */
protected static $core = [];

// ...
$id = get_current_blog_id();
self::$interface[$id] = new H5PWordPress();
$language = $this->get_language();
self::$core[$id] = new H5PCore(self::$interface[$id], $this->get_h5p_path(), $this->get_h5p_url(), $language, get_option('h5p_export', TRUE));
self::$core[$id]->aggregateAssets = !(defined('H5P_DISABLE_AGGREGATION') && H5P_DISABLE_AGGREGATION === true);
/// ...

Would require modifications to a dozen other places in the class. Brainstorming here. Better ideas welcome. Yes to an optional variable to reset or set a custom path, if possible to fix both paths more simply.

@dac514
Copy link

dac514 commented Nov 22, 2018

Would require modifications to a dozen other places in the class.

Actually less work than I thought. They were all in the same function. This works for me?

/**
 * @var \H5PWordPress[]
 */
protected static $interface = [];

/**
 * @var \H5PCore[]
 */
protected static $core = [];

// ...

  /**
   * Get the different instances of the core.
   *
   * @since 1.0.0
   * @param string $type
   * @return \H5PWordPress|\H5PCore|\H5PContentValidator|\H5PExport|\H5PStorage|\H5PValidator
   */
  public function get_h5p_instance($type) {
    $id = get_current_blog_id();
    if (empty(self::$interface[$id])) {
      self::$interface[$id] = new H5PWordPress();
      $language = $this->get_language();
      self::$core[$id] = new H5PCore(self::$interface[$id], $this->get_h5p_path(), $this->get_h5p_url(), $language, get_option('h5p_export', TRUE));
      self::$core[$id]->aggregateAssets = !(defined('H5P_DISABLE_AGGREGATION') && H5P_DISABLE_AGGREGATION === true);
    }

    switch ($type) {
      case 'validator':
        return new H5PValidator(self::$interface[$id], self::$core[$id]);
      case 'storage':
        return new H5PStorage(self::$interface[$id], self::$core[$id]);
      case 'contentvalidator':
        return new H5PContentValidator(self::$interface[$id], self::$core[$id]);
      case 'export':
        return new H5PExport(self::$interface[$id], self::$core[$id]);
      case 'interface':
        return self::$interface[$id];
      case 'core':
        return self::$core[$id];
    }
  }

icc added a commit that referenced this issue Dec 18, 2018
@icc
Copy link
Member

icc commented Dec 18, 2018

Doesn't appear to do any harm so I've added it.

Let me know if you find any issues using it.

@dac514
Copy link

dac514 commented Dec 19, 2018

Any thoughts on:

get_h5p_url has a static $url; in it. I could not find a trivial way to reset it.

The problem is this line:

self::$core[$id] = new H5PCore(self::$interface[$id], $this->get_h5p_path(), $this->get_h5p_url(), $language, get_option('h5p_export', TRUE));

Pseudo example:

switch_to_blog(1);
$a = $foo->get_h5p_instance('core');
var_dump($a->url);
switch_to_blog(2);
$b = $foo->get_h5p_instance('core');
var_dump($b->url);

Expected: $a and $b have different URLs.
Actual: $a and $b have the same URLs.

icc added a commit that referenced this issue Dec 20, 2018
@icc
Copy link
Member

icc commented Dec 20, 2018

I've added the same fix here as it seems this was the only other static that should affect this.

@SteelWagstaff
Copy link
Author

@icc FYI, we ran into a new issue in our testing: #83

@SteelWagstaff
Copy link
Author

This feature was included in the 1.13.0 release of the H5P for WordPress plugin. Support for H5P cloning will be included in the 5.7.0 release of Pressbooks. Thanks to everyone who worked on this!

@SteelWagstaff
Copy link
Author

@icc We've just noticed that licensing information ('rights of use') attached to activities aren't being included when activities are cloned/copied. This is of course a problem for anyone who is planning to remix/revise activities while respecting the original Creative Commons license. Is this information being made available to REST endpoints? If not, would you be willing to add these, or support a PR adding them?

@icc
Copy link
Member

icc commented Jan 29, 2020

It looks as if the appropriate metadata properties are missing from fetch_h5p: public/class-h5p-plugin.php#L1526.
I don't think this should be difficult to fix, but if you make a PR for it that would be great and also we'll be sure that it works for your use as well.

@cagp-dev-mtl
Copy link
Contributor

Hello @icc , I have submitted a PR with the fix, please refer PR:

#110

Thank you

@icc
Copy link
Member

icc commented Feb 10, 2020

Thank you. I've merged it in and it will be part of the next plugin release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants