Skip to content
This repository has been archived by the owner on Apr 2, 2020. It is now read-only.

Add PDF.js to the repo #193

Closed
wants to merge 1 commit into from
Closed

Conversation

rmccue
Copy link
Contributor

@rmccue rmccue commented Nov 16, 2016

Fixes #192.

This is pretty huge, so might warrant splitting into a separate plugin at this point, tbh. Figured I'd file the PR in any case. :)

@davisshaver
Copy link
Member

+1 to merging this. I have a cleaned up version almost ready to go through CI, but needs a look at these two test cases.

public function test_https_respect() {
	$post_id = $this->factory->post->create( array( 'post_content' => '[pdf url="https://www.gpo.gov/fdsys/pkg/BILLS-114hr2048enr/pdf/BILLS-114hr2048enr.pdf"]' ) );
	$post = get_post( $post_id );
	$this->assertContains( 'https://mozilla.github.io/pdf.js/web/viewer.html?file=' . rawurlencode( 'https://www.gpo.gov/fdsys/pkg/BILLS-114hr2048enr/pdf/BILLS-114hr2048enr.pdf' ), apply_filters( 'the_content', $post->post_content ) );
}

public function test_http_respect() {
	$post_id = $this->factory->post->create( array( 'post_content' => '[pdf url="http://www.gpo.gov/fdsys/pkg/BILLS-114hr2048enr/pdf/BILLS-114hr2048enr.pdf"]' ) );
	$post = get_post( $post_id );
	$this->assertContains( 'http://mozilla.github.io/pdf.js/web/viewer.html?file=' . rawurlencode( 'http://www.gpo.gov/fdsys/pkg/BILLS-114hr2048enr/pdf/BILLS-114hr2048enr.pdf' ), apply_filters( 'the_content', $post->post_content ) );
}

So for one we'll have a different PDF URL than the mozilla one, but how should we handle http respect? Is there a way to mock SSL in the test and expect https?

@goldenapples
Copy link
Contributor

how should we handle http respect? Is there a way to mock SSL in the test and expect https?

I don't think we should need to mock anything there for tests, since the tests aren't actually making an HTTPS request, just checking that the markup returned by the callback includes an iframe with an https src. The shortcode callback will need to be updated to handle that, but I'm not sure if checking the protocol of the PDF is the best way to do that - trying to serve an https iframe from a site without a valid ssl certificate will cause problems.

I'd like to see some messaging added about the CORS issues involved - trying to test it out I couldn't find any public pdfs online with CORS headers, so I had to download one and upload it to test this out.

I do like the idea about a CORS proxy as mentioned on the issue, as long as it was optional and could be disabled by a filter. It might be a good idea to expose an attribute on the shortcode as a checkbox which would run the PDF through a local proxy.

I'm not a huge fan of the size of this PR, but I don't know of any standard ways of handling package dependencies in WP plugins. If anyone has a preference for ways to include this as a package (git submodule seems to make the most sense to me, but I see composer being used for this also) rather than checking all 367 files into this repo, I'd say go for it. We'd have to check them in to svn in the plugin repo, but at least the diffs wouldn't be so huge here.

@goldenapples
Copy link
Contributor

ways to include this as a package

Ah, I guess I see why you didn't go this route in the first place. Looks like there's two possible packages that could be pulled from - the pdfjs-dist repo, which includes builds files of the basic script files and stylesheets required, but not the html template, and the gh_pages branch of the PDF.js repo, which has a /web directory containing basically the files checked in here. Unfortunately, checking this in as a submodule would require checking all of the other directories of the gh_pages branch in, which is just as unwieldy as what's checked in here.

@davisshaver
Copy link
Member

@goldenapples Agree with your reasoning. CORS Proxy would be nice but I also think we could handle it outside scope of this PR. I looked into the repo options with PDF.js and agree with your analysis, the directory we have right now from gh-pages is probably as lightweight we can get. There's an NPM module we could use, that probably doesn't make things any easier either.

I have another PR in progress that updates the test cases, perhaps we can continue the conversation there? #197

@goldenapples
Copy link
Contributor

There's an NPM module we could use, that probably doesn't make things any easier either.

Yeah, looks like that module is just the pdfjs-dist module, which isn't quite enough to make this work, since it doesn't include the viewer that we want to use. I like the idea of including the example docs as a submodule from the gh-pages branch of PDF.js, but that would require some svn hackery when we want to check it into the plugin repo.

we can continue the conversation there? #197

👍 I'll close this PR for now and move discussion over there.

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

Successfully merging this pull request may close these issues.

None yet

3 participants