I think it is bad style to have a module depend on PHP filter. PHP filter is a security hazard and should not be used if at all possible. If a site builder makes a decision to not use it, this module shouldn't force him to re-enable it.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

killua99’s picture

Thanks for the information. This issue have a lot post. I don't like PHP filter at all.

In the new version 2 will be removed. But this one was a old branch I start working so it's legacy code. I cannot simple remove it.

So I'll close this issue because it's a know bug.

killua99’s picture

Status: Active » Closed (duplicate)
emattias’s picture

Here's a patch for those that dont want to enable the PHP module just to use views_pdf

emattias’s picture

Apparently my new git diff settings mangled the patch. Here's the #3 patch, non-mangled. :)

killua99’s picture

Yes, but figure out we have an even worse method with pure eval that don't need PHP module to be enable.

Anyways, you approach is quite interesting. Lets open a diff bugg with theses patchs.

ELC’s picture

Title: Drop dependency on PHP filter » Drop dependency on PHP filter, make it optional
Status: Closed (duplicate) » Needs review
FileSize
10.05 KB

I have to concur with killes and emattias. Having the PHP module as a forced dependency is unnecessary and a security risk. It also causes issues for those of us who do not allow the php module.

The attached patch includes guards around a couple of places in views_pdf_template.php where the actual execution takes place.

This patch should be included without finding an alternative method to do the same functionality. All it does is make the inclusion of the php module optional while maintaining existing functionality on sites who wish to enable it. Existing sites will continue unaffected, while new installs that wish to take advantage of the php module, can do so by manually enabling the module.

If this is a "known bug" then it should be made very clear somewhere that it exists and the work around to avoid it. In my mind, this issue does not warrant ignoring it as a such. The php module is deleted on all sites I host and build, meaning that I must patch this module before it can be used. A better solution would be create two hooks which would all developers to hook into the same locations without needing the php module.

I was unable to locate the "open" issue for this problem, and so I have re-opened this one. Please link to the duplicate issue(s) when closing an issue as duplicate so that people can easily make it to the correct issue.
eg

(These probably aren't actually the duplicate issues)

ELC’s picture

Sorry about that. I managed to fluff up the code when going from a hacked in 1.3 version to the dev version of this patch.

I have disabled the display of the two fluffed up patches in this issue.

vegansupreme’s picture

Works for me.

killua99’s picture

@vegansupreme did you test it well?

I'll try to test as soon as I can (soon I'm starting my summer break, but I'm not gonna use it to code) seen a solid solution with the code. This make optional the PHP module, but don't avoid use it. Anyways is better than today.

vegansupreme’s picture

@Killua, I suppose I tested it moderately well.
One thing I noticed about this patch is if PHP Filter module is disabled, it disables both PHP options—php_eval, and eval. Without PHP filter, (unless I'm mistaken) one could still be able to use eval. It is probably better for security to do it this way though. That way if PHP filter is disabled or uninstalled, there is no way of entering any PHP into Views PDF. I can still use my eval'd PHP layout code with this patch applied as long as PHP filter enabled.

killua99’s picture

I see your point.

I'll make a fix to don't hide the "eval" anyways is a PHP function and don't have any relation with the drupal php_eval.

Still I'm looking for a good way to avoid using eval (any) for the next version.

killua99’s picture

Assigned: Unassigned » killua99
Category: Bug report » Feature request
Status: Needs review » Needs work
vegansupreme’s picture

Just guessing, based on how I use the module, I'd suspect most people using PHP are using it for layout—2 column, or 3 column. If we can get those layouts into the next version many people using PHP in their views will no longer have to. There will still be people that want to tap into the advanced features of TCPDF, such as barcodes and QR codes. Some may also want to use conditionals in PHP to create custom layouts. Some may want to use PHP to access site variables (current user, date etc).

What's the best way to allow users to access this advanced functionality?

  • Have an included module that adds the PHP eval text areas?
  • Have a separate module to add PHP eval functionality?
  • Use hooks, documentation and an example module so users can insert PHP into their view using custom modules?
killua99’s picture

Yeah for that and border.

You thinking you can start some work how to handle the 2 - 3 column layout? Maybe if you start something in this version I can port that for the V2 or you can start directly with the V2. You know the repo, but the way how I'm building this module is more D8 way, so if you need some hint we can schedule a meeting in IRC and I'll explain to you more about how develop with this method.

And to grant access to advance settings maybe we should include some hooks or code include. Then the PHP will be on code, and not in DB or inside views directly.

vegansupreme’s picture

I've started on 2–3 col layout, but didn't get very far. I got a little stumped, but then hadn't had much time to figure it out. I'll email you about IRC times.

killua99’s picture

Cool wait for it

bradjones1’s picture

Status: Needs work » Needs review
FileSize
9.48 KB

Rerolled for latest dev.

killua99’s picture

+++ b/views_pdf_plugin_row_fields.inc
@@ -265,30 +265,31 @@ class views_pdf_plugin_row_fields extends views_plugin_row {
+      if (module_exists('php')) {

Maybe we create a constant or definition for this. I'm worry about performance here.

For the rest look good enough still need more review, vegansupreme is more active than me this days.

vegansupreme’s picture

This patch works for me. If PHP filter is disabled, I can't enter any PHP at all, but when PHP Filter is enabled, I have the choice of php_eval or plain eval. This seems to be the desired effect, even though you could still use eval without the PHP filter module. The point is to improve security. If I want to use eval, I have to enable PHP Filter.

Thanks Killua99 for input about performance. @bradjones, would you be able to create a constant or definition, as Killua99 suggests, and post a new patch?

vegansupreme’s picture

This patch creates an option variable for module_exists('php').
Also, it adds a user permission to allow PHP code in View PDF. This gives another option to disable user entry of PHP, without blocking the execution of it.
Disabling PHP will disable both entry and execution of PHP code.
Finally, it adds PHP functionality to table PDF format. The functionality was already there on the render side, just not the UI side. This is also conditional on the permissions and the PHP module.
Does this look good Killua99?

killua99’s picture

Assigned: killua99 » vegansupreme
Status: Needs review » Needs work
  1. +++ b/views_pdf_plugin_row_fields.inc
    @@ -264,30 +263,31 @@ class views_pdf_plugin_row_fields extends views_plugin_row {
    +      if (user_access('allow php in views pdf') && module_exists('php')) {
    

    I'm talking about put this on top on views_pdf.module

    define('VIEWS_PDF_PHP', (user_access('allow php in views') && module_exists('php'));

    or split it.

  2. +++ b/views_pdf_template.php
    @@ -257,10 +256,18 @@ class PdfTemplate extends FPDI {
    +    if (module_exists('php')) {
    +      $options['render']['php_status'] = 1; // User PHP code will execute.
    +    }
    +    else {
    +      $options['render']['php_status'] = 0;
    +    }
    +
    

    What about;

    $options['render']['php_status'] = 0;

    if (VIEWS_PDF_PHP) {
    $options[render][php_status] = 1
    }

  3. +++ b/views_pdf_template.php
    @@ -505,11 +512,13 @@ class PdfTemplate extends FPDI {
    +        if ($options['render']['php_status'] == 1) {
    

    For PHP runtime is better use: if (xxx === yyy) // (3 eq)

killua99’s picture

@vegansupreme Now I'm lost where to look on the issue queue. Can you point me the top 3 most important issue at the moment? Thanks.

I'll resume the development v2, how's going the table thing? did you commit something to github ? Can you make a push request on github if you have something?

vegansupreme’s picture

@Killua99 I think splitting it would be the best option. The permission should only be for inputing PHP into the admin UI. Non admins should still be able to view a PDF with PHP, unless php_filter is disabled—then no one gets any PHP!

I knew you were going to say use === ;-) I hope to have a new patch up soon!

Glad to hear you're getting back into the v2 branch development! This was our last discussion about v2:
https://www.drupal.org/node/2292621#comment-9307869

As for columns in v2, I got something working, and committed to my fork, but it's still messy. I posted about it here:
https://www.drupal.org/node/1345182#comment-9181201

killua99’s picture

Alright ...

I see someone ask for a Task list, I see a lot people hack this module (I know why) and some are talking about Drupal 8.

I'll try to add more info for the V2, cause is needed.

What I know is; V2 need to be solid and multi library + Drupal 8 is around the corner, we need to push this forward and finish the v2 for D7 then start working for D8. With this last Beta 6 look more solid then ever so is time to start with D8.

I'm surprise about 6k installations. When I start was 3k

About this patch I'm agree to split the definitions.

vegansupreme’s picture

FileSize
12.67 KB

New patch attached. I had looked into doing a constant definition, but wasn't sure where to put it. Thanks for your guidance—it simplified the code too. I no longer have to use the $options['render']['php_status'] at all.

For v2 I think getting the code cleaned up, and into modern standards will be a big help. It's hard debugging this code!

killua99’s picture

Assigned: vegansupreme » Unassigned
Status: Needs work » Reviewed & tested by the community

Ok nice, the code look "nice" you know, is really messy xD

Commit it when you can.

  • vegansupreme committed a7ebcef on 7.x-1.x
    Issue #2126931 by vegansupreme, ELC, emattias, bradjones1, killes@www....
vegansupreme’s picture

Status: Reviewed & tested by the community » Fixed

Committed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.