Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#25 | 2126931-25-remove-php-module-dependency.patch | 12.67 KB | vegansupreme |
#17 | 2126931-17-remove-php-module-dependency.patch | 9.48 KB | bradjones1 |
#7 | 2126931-7-remove-php-module-dependency.patch | 10.04 KB | ELC |
#4 | 2126931-4-remove-php-module-dependency.patch | 7.7 KB | emattias |
Comments
Comment #1
killua99 CreditAttribution: killua99 commentedThanks 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.
Comment #2
killua99 CreditAttribution: killua99 commentedComment #3
emattias CreditAttribution: emattias commentedHere's a patch for those that dont want to enable the PHP module just to use views_pdf
Comment #4
emattias CreditAttribution: emattias commentedApparently my new git diff settings mangled the patch. Here's the #3 patch, non-mangled. :)
Comment #5
killua99 CreditAttribution: killua99 commentedYes, 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.
Comment #6
ELC CreditAttribution: ELC commentedI 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)
Comment #7
ELC CreditAttribution: ELC commentedSorry 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.
Comment #8
vegansupreme CreditAttribution: vegansupreme commentedWorks for me.
Comment #9
killua99 CreditAttribution: killua99 commented@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.
Comment #10
vegansupreme CreditAttribution: vegansupreme commented@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.
Comment #11
killua99 CreditAttribution: killua99 commentedI 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.
Comment #12
killua99 CreditAttribution: killua99 commentedComment #13
vegansupreme CreditAttribution: vegansupreme commentedJust 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?
Comment #14
killua99 CreditAttribution: killua99 commentedYeah 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.
Comment #15
vegansupreme CreditAttribution: vegansupreme commentedI'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.
Comment #16
killua99 CreditAttribution: killua99 commentedCool wait for it
Comment #17
bradjones1Rerolled for latest dev.
Comment #18
killua99 CreditAttribution: killua99 commentedMaybe 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.
Comment #19
vegansupreme CreditAttribution: vegansupreme commentedThis 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?
Comment #20
vegansupreme CreditAttribution: vegansupreme commentedThis 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?
Comment #21
killua99 CreditAttribution: killua99 commentedI'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.
What about;
$options['render']['php_status'] = 0;
if (VIEWS_PDF_PHP) {
$options[render][php_status] = 1
}
For PHP runtime is better use: if (xxx === yyy) // (3 eq)
Comment #22
killua99 CreditAttribution: killua99 commented@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?
Comment #23
vegansupreme CreditAttribution: vegansupreme commented@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
Comment #24
killua99 CreditAttribution: killua99 commentedAlright ...
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.
Comment #25
vegansupreme CreditAttribution: vegansupreme commentedNew 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!
Comment #26
killua99 CreditAttribution: killua99 commentedOk nice, the code look "nice" you know, is really messy xD
Commit it when you can.
Comment #28
vegansupreme CreditAttribution: vegansupreme commentedCommitted.