Implement WP_Filesystem for certificates and certificate previews for VIP #161

rinatkhaziev · 2018-11-06T03:55:23Z

This PR addresses an incompatibility with VIP Go file system implementation. Due to the fact that Go runs in a containerized environment all uploads are offloaded to a CDN, and the files are not present physically in the file system.

To work that around a new child class is introduced WP_tFPDF, a thin wrapper that re-implements Output method, and introduces ImageWrapper method.

ImageWrapper gets passed the file as a string, makes a write to OS temporary folder, and passes the filename so that image could be properly processed and attached to the generated PDF.

Additionally, fixes an issue with JS throwing errors when trying to draw zones for certificate data.

…e filesystem Fix an issue with editable areas being broken in Certificate templates

…ntroducing WP_tFPDF::ImageWrapper that gets passed the file contents as a string, makes a write to a system's temporary folder, and then passes the filename of the temporary file to tFPDF::Image

alexsanford

@rinatkhaziev Great work on this! I've tested it out and it seems to work really well!

I have a few questions/comments on the implementation. Particularly since we'll have to maintain this moving forward, so I want to make sure it's nice and clean, and also future-proof (i.e. so it's clear what needs to happen if/when we update or replace the tFPDF library).

I hope my comments make sense. Please feel free to reach out with any questions.

lib/tfpdf/wp-tfpdf.php

alexsanford · 2018-11-08T01:39:33Z

lib/tfpdf/wp-tfpdf.php

+			header('Pragma: public');
+			echo $this->buffer;
+			break;
+		case 'F':


@rinatkhaziev How did you test this case? I see that this is the code that was changed here from the original implementation, but I'm not seeing any way that this code can actually be reached from the Certificates code.

I tested this by adding a call to generate_pdf( wp_upload_dir()['path'] ) on a WooThemes_Sensei_PDF_Certificate instance. I added this in classes/class-woothemes-sensei-certificates.php underneath the existing call to generate_pdf(). This seemed to generate the file just fine.

Correct, there's no obvious code path to get the file to be written. The one I used is actually calling the method inside a sensei_certificates_before_pdf_output action hook.

In all honesty, this can probably be removed without anyone noticing. I just didn't want to trample on the flowers in someone else's garden :)

classes/class-woothemes-sensei-certificate-templates.php

classes/class-woothemes-sensei-pdf-certificate.php

lib/tfpdf/wp-tfpdf.php

alexsanford · 2018-11-08T02:08:47Z

lib/tfpdf/wp-tfpdf.php

+		if ( ! is_a( $wp_filesystem, 'WP_Filesystem_Base' ) ) {
+			$creds = request_filesystem_credentials( site_url() );
+			$fs = wp_filesystem( $creds );
+			return $fs ?: new WP_Error( 'fs-init-error', "Couldn't initialize Filesystem" );


Maybe a regular if statement would be a little clearer here?

Also, we never actually use this return value. Maybe it would be better to throw an exception instead?

I'm also not sure we need to return true below.

So the logic goes, if the $wp_filesystem is not initialized, try to do so. wp_filesystem( $creds ) returns boolean TRUE/FALSE. If it's FALSE, return a WP_Error.

In case the filesystem is initialized properly - just return TRUE.

As for Elvis operator instead of a regular if/else is a personal preference, I think it looks cleaner than nested ifs.

Re: initialization - I've addressed this workflow in my latest review.

Re: Elvis operator - this will probably be going away anyway, but in case it doesn't, I think we should remove the use of the Elvis operator. I don't mind it personally, but it's unconventional as far as our coding standards go, and it's not supported in PHP 5.2. Note that Jetpack discourages its use.

alexsanford · 2018-11-08T02:11:02Z

lib/tfpdf/wp-tfpdf.php

+		$this->init_wp_filesystem();
+	}
+
+	function init_wp_filesystem() {


Should this be private?

No particular reason for it to be or not to be private, since it operates on a global $wp_filesystem

I'm thinking that this method shouldn't be used by 3rd-party code, and should only be called from the constructor. So making it private is probably better from a maintenance standpoint. That way, if we want to change it, we don't have to worry about other code that might be using it for some reason 🙂

lib/tfpdf/wp-tfpdf.php

classes/class-woothemes-sensei-certificates.php

classes/class-woothemes-sensei-pdf-certificate.php

…e Image method, do the necessary logic there before passing to tFPDF::Image Remove trailing whitespaces Convert tFPDF to WPCS (for the most part, except the UpperCamelCase method names

…arty dependency but our wrapper around it. Fix a couple of missed CS violations

alexsanford · 2018-11-08T15:32:20Z

classes/class-wp-tfpdf.php

+		$this->init_wp_filesystem();
+	}
+
+	function init_wp_filesystem() {


I haven't used WP_Filesystem before, so I'm digging in and trying to understand how this will work for our users.

From my reading and testing, it seems that this initialization method will work fine in the following two cases:

We are using the direct method to access the filesystem.

We have the FTP creds stored in constants (e.g. in wp-config.php).

However, this does not work in the case where we need to prompt for credentials. In this case, we would need the prompt to happen in the context of a POST request so we can do the redirection properly. Unfortunately, at this point we do not have this POST request since there is no actual code path within the plugin that performs the PDF creation.

I wonder if the following would be a better solution:

Do not use WP_Filesystem by default (i.e. go back to using the regular tFPDF class).

Have a filter (and document it!) that enables the use of this subclass instead (perhaps sensei_certificiates_use_wp_filesystem).

Inside this class, do not initialize WP_Filesystem. Instead, add documentation to the generate_pdf method and/or the new filter specifying that 3rd-party code wanting to use WP_Filesystem needs to make sure it is initialized. That way it can be done at the level of the POST request, and they can properly handle the intricacies of the initialization.

This way, VIP sites could enable the filter and handle the initialization. I'm assuming that typical VIP have the creds stored in constants? In which case, using a solution such as the one you wrote should work fine, you would just have to add it to hte individual site(s).

Does this make sense? Any thoughts? I'm just very reluctant to add code to enable WP_Filesystem by default without making it work in all cases, and I don't see how we can properly initialize it in this plugin...

alexsanford · 2018-11-08T15:35:30Z

lib/tfpdf/wp-tfpdf.php

+		if ( ! is_a( $wp_filesystem, 'WP_Filesystem_Base' ) ) {
+			$creds = request_filesystem_credentials( site_url() );
+			$fs = wp_filesystem( $creds );
+			return $fs ?: new WP_Error( 'fs-init-error', "Couldn't initialize Filesystem" );


Re: initialization - I've addressed this workflow in my latest review.

Re: Elvis operator - this will probably be going away anyway, but in case it doesn't, I think we should remove the use of the Elvis operator. I don't mind it personally, but it's unconventional as far as our coding standards go, and it's not supported in PHP 5.2. Note that Jetpack discourages its use.

alexsanford

This is looking a lot better!

The only blocker for me now is the WP_Filesystem initialization. It's still a step in the right direction, but it's not going to work in all cases. Please have a look at my comment.

… only, forgoing broader effort to re-implement all filesystem-related logic using WP_Filesystem: Rename WP_tFPDF into VIP_tFPDF to clearly signal that the file is VIP-compat Rename the filter to sensei_certificates_vip_compat

rinatkhaziev · 2018-11-27T19:28:20Z

After internal discussion it was decided to limit the scope of this work to only address VIP compatibility issues, forgoing the effort of a broader refactoring to implement WP_Filesystem.

To enable WP_Filesystem this code needs to be added either in VIP Platform or userland code:

		add_filter( 'sensei_certificates_vip_compat', '__return_true' );

alexsanford

I think this looks good! I'd like a second opinion from @jom on the approach we took here, but otherwise I think this is good to go!

FYI @jom for some reason your commit (0eaf685) didn't work on VIP Go, so it isn't included here. It's probably not needed, since we're scoping this to be VIP-specific code.

jom · 2018-11-28T12:36:03Z

I think ideally we'd add a filter on the tFPDF class to use and put VIP_tFPDF inside a VIP only side-plugin, but the code is dead-enough on non-VIP that I don't think it is a blocker if this needs to get in. If we ship VIP specific code inside the plugin, could we just use VIP_GO_ENV or some other sniff to see if we are in VIP Go-land?

alexsanford · 2018-11-28T13:50:24Z

I think ideally we'd add a filter on the tFPDF class to use and put VIP_tFPDF inside a VIP only side-plugin

I had suggested this to @rinatkhaziev in our discussion, but the feeling was that this would cause extra friction for our users, and that in general we should try to make A8c plugins "just work" for VIP. Explicitly scoping this to code to VIP seemed like the best compromise.

If we ship VIP specific code inside the plugin, could we just use VIP_GO_ENV or some other sniff to see if we are in VIP Go-land?

Hmm. This would be more explicit and clear, but would make it trickier for others to also use this code if desired. That said, we have explicitly named the filter based on vip compatibility...

@rinatkhaziev What do you think?

…VIP_ENV instead.

rinatkhaziev · 2018-11-28T17:46:56Z

@jom is right, we do have a constant, it's called WPCOM_IS_VIP_ENV. And since we're limiting the scope to VIP compat only, it makes sense to use it. I removed the filter and put a check for that constant instead.

As @alexsanford mentioned, we think putting this functionality in a VIP-only side-plugin for an extension for an A8c's plugin is a bit sub-optimal :) It's easy to imagine a situation where the extension itself would be updated with breaking changes and nobody will remember to update this side-plugin.

alexsanford · 2018-11-28T18:13:27Z

@rinatkhaziev This looks great! Thanks for all your hard work on this one!

) * Implement WP_Filesystem when trying to generate and write a PDF to the filesystem Fix an issue with editable areas being broken in Certificate templates * Fix broken images in the certificate template preview for VIP Go by introducing WP_tFPDF::ImageWrapper that gets passed the file contents as a string, makes a write to a system's temporary folder, and then passes the filename of the temporary file to tFPDF::Image * Address feedback: remove WP_tFDPF::ImageWrapper , instead add back the Image method, do the necessary logic there before passing to tFPDF::Image Remove trailing whitespaces Convert tFPDF to WPCS (for the most part, except the UpperCamelCase method names * Address feedback: move wp-tfpdf.php out of lib since it's not a 3rd-party dependency but our wrapper around it. Fix a couple of missed CS violations * Limiting the scope of the update to implementing VIP Go compatibility only, forgoing broader effort to re-implement all filesystem-related logic using WP_Filesystem: Rename WP_tFPDF into VIP_tFPDF to clearly signal that the file is VIP-compat Rename the filter to sensei_certificates_vip_compat * Remove the sensei_certificates_vip_compat filter, rely on a WPCOM_IS_VIP_ENV instead.

rinatkhaziev added 2 commits November 5, 2018 21:01

Implement WP_Filesystem when trying to generate and write a PDF to th…

f80456b

…e filesystem Fix an issue with editable areas being broken in Certificate templates

Fix broken images in the certificate template preview for VIP Go by i…

e5c906c

…ntroducing WP_tFPDF::ImageWrapper that gets passed the file contents as a string, makes a write to a system's temporary folder, and then passes the filename of the temporary file to tFPDF::Image

donnapep requested a review from alexsanford November 6, 2018 14:26

donnapep added this to the 1.1.1 milestone Nov 6, 2018

alexsanford reviewed Nov 8, 2018

View reviewed changes

rinatkhaziev added 2 commits November 7, 2018 22:49

Address feedback: remove WP_tFDPF::ImageWrapper , instead add back th…

493872a

…e Image method, do the necessary logic there before passing to tFPDF::Image Remove trailing whitespaces Convert tFPDF to WPCS (for the most part, except the UpperCamelCase method names

Address feedback: move wp-tfpdf.php out of lib since it's not a 3rd-p…

737ad83

…arty dependency but our wrapper around it. Fix a couple of missed CS violations

alexsanford reviewed Nov 8, 2018

View reviewed changes

alexsanford requested changes Nov 8, 2018

View reviewed changes

alexsanford mentioned this pull request Nov 19, 2018

Implement WP_Filesystem for certificates and certificate previews. #163

Closed

alexsanford requested a review from jom November 27, 2018 20:42

alexsanford approved these changes Nov 27, 2018

View reviewed changes

Remove the sensei_certificates_vip_compat filter, rely on a WPCOM_IS_…

46abead

…VIP_ENV instead.

alexsanford merged commit ba8d073 into woocommerce:master Nov 28, 2018

donnapep changed the title ~~Implement WP_Filesystem for certificates and certificate previews.~~ Implement WP_Filesystem for certificates and certificate previews for VIP Nov 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement WP_Filesystem for certificates and certificate previews for VIP #161

Implement WP_Filesystem for certificates and certificate previews for VIP #161

rinatkhaziev commented Nov 6, 2018

alexsanford left a comment

alexsanford Nov 8, 2018

rinatkhaziev Nov 8, 2018

alexsanford Nov 8, 2018

rinatkhaziev Nov 8, 2018

alexsanford Nov 8, 2018

alexsanford Nov 8, 2018

rinatkhaziev Nov 8, 2018

alexsanford Nov 8, 2018

alexsanford Nov 8, 2018

alexsanford Nov 8, 2018

alexsanford left a comment •

edited

rinatkhaziev commented Nov 27, 2018

alexsanford left a comment

jom commented Nov 28, 2018 •

edited

alexsanford commented Nov 28, 2018

rinatkhaziev commented Nov 28, 2018

alexsanford commented Nov 28, 2018

Implement WP_Filesystem for certificates and certificate previews for VIP #161

Implement WP_Filesystem for certificates and certificate previews for VIP #161

Conversation

rinatkhaziev commented Nov 6, 2018

alexsanford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexsanford left a comment • edited

Choose a reason for hiding this comment

rinatkhaziev commented Nov 27, 2018

alexsanford left a comment

Choose a reason for hiding this comment

jom commented Nov 28, 2018 • edited

alexsanford commented Nov 28, 2018

rinatkhaziev commented Nov 28, 2018

alexsanford commented Nov 28, 2018

alexsanford left a comment •

edited

jom commented Nov 28, 2018 •

edited