clonemeagain / attachment_preview

osTicket Plugin: Allows inline view of attachments
GNU General Public License v2.0
48 stars 16 forks source link

bug: Parse error: syntax error, unexpected '.', expecting '&' #21

Closed mckaygerhard closed 6 years ago

mckaygerhard commented 6 years ago

i got and error when try to install this attachment preview:

it said: Successfully installed a plugin but get and error:

Parse error: syntax error, unexpected '.', expecting '&' or variable (T_VARIABLE) in /var/share/www-data/osticket/include/plugins/attachment_preview/class.AttachmentPreviewPlugin.php on line 647
mckaygerhard commented 6 years ago

firts time i see that: xxx($var1, ....$var2) some dots here! the error are throwing by that

mckaygerhard commented 6 years ago

well i think this can be solved based on documentation of stupid php 5.6:

--- a/include/plugins/attachment_preview/class.AttachmentPreviewPlugin.php
+++ b/include/plugins/attachment_preview/class.AttachmentPreviewPlugin.php
@@ -644,7 +644,7 @@ class AttachmentPreviewPlugin extends Plugin {
     /**
      * Wrapper around log method
      */
-    private function debug_log(...$args) {
+    private function debug_log($args) {
         if (self::DEBUG) {
             $text = array_shift($args);
             $this->log($text, $args);
@@ -657,12 +657,12 @@ class AttachmentPreviewPlugin extends Plugin {
      * @param string $text
      * @param mixed $args
      */
-    private function log($text, ...$args) {
+    private function log($text, $args=null) {
         // Log to system, if available
         global $ost;

-        if (func_num_args() > 1) {
-            $text = vsprintf($text, ...$args);
+        if (is_array($args) AND count($args > 1) ) {
+            $text = vsprintf($text, $args);
         }

         if (!$ost instanceof osTicket) {
clonemeagain commented 6 years ago

Hey chief,

Thanks for your work! I'm stretched at the office right now, but I'm having a look.

... is no longer a syntax "error", but the relatively new (3+ years) variadic expansion operator or "splat", introduced in PHP 5.6, which is the lowest supported PHP version since osTicket 1.7+. (You might have found that already). See osticket.com/download page under "System Requirements".

It does look like I've used it wrong the second time though, the generated array of arguments from the variadic declaration should have been passed to vsprintf as an array, not unpacked into vsprintf via the variadic unpack operator... hmm. If we're unpacking, then it should be sprintf, not vsprintf or, just not unpack at all. However, if we're not unpacking, then do we need the variadics? Just unshift the first arg the "normal way". Which we do for debug_log, but not log. Ahh. I see now, if debug_log packs the arguments into an array (func_get_args or splat), then passes the array to log, log then has to unpack it.. tricky.

How about this instead:

  /**
     * Wrapper around log method, same signature/result, only logs when DEBUG is true.
     */
    private function debug_log($text, $_=null) {
        if (self::DEBUG) {
            $args = func_get_args();
            $text = array_shift($args);
            $this->log($text, $args); // send variable amount of args as array
        }
    }

    /**
     * Supports variable replacement of $text using sprintf macros
     *
     * @param string $text with sprintf macros
     * @param $_ any number of params for the macros
     */
    private function log($text, $_=null) {
        // Log to system, if available
        global $ost;

        $args = func_get_args(); 
        $format = array_shift($args);
        if(!$format){
            return;
        }
        if(is_array($args[0])){ // handle debug_log or array of variables
            $text = vsprintf($format,$args[0]);
        }elseif(count($args)){ // handle normal variables as arguments
            $text = vsprintf($format,$args);
        }else{// no variables passed
            $text = $format;
        }
... <rest of log>...

While it's possible to support earlier versions (by changing the structure of that function etc), it seems wrong, 5.3 is ancient!. http://php.net/supported-versions.php. 5.6 is the final release of PHP5, and it is "security support only" for another 11 months, it will be end of life by Jan 2019. Should we try and support 4? I'm of the opinion that supporting earlier versions should not be done. But please, let me know why you think we should.

From a security/admin perspective: I'd rather encourage everyone to upgrade. The newer PHP's are more secure, faster and more efficient, therefore costing less to run in terms of CPU/Energy etc.

From a programming perspective: I want to use the new features! Learning new stuff, but rarely get to use it! Very frustrating to be stuck writing in a 5.3 version of the language. A contrived non-IT metaphor would be: if this post was written in Latin. :-)

I concede however, that the people using osTicket are likely more important than me standing on a hill of PHP7 by myself.. can you test the above in 1.9? It works in 1.10 with PHP7. :-)

mckaygerhard commented 6 years ago

hi @clonemeagain you are wrong the lowest version ARE 5.2 for osticket 1.7 , 5.3 for osticket 1.8 or 1.9 and 5.6 for osticket 1.10, i 'm using osticket 1.7 in php 5.2 and the instruction by searching in google cache or archive net said at that time 5.3 as minimun... that all by this part

about the second, just more easy, convert implicit tha argument into and array and that's all.. i'm flavored of simpel solutions event too complicated thing, apart that the funtion only wwrite log messages..

please commit the pull or improve it, due by that many vendors cannot use it.. php 5.3 and 5.2 are still used patched for security..

a special note: that stupidity about "old version unsuported" the word "unsuporte" are miscpelled, corrected must be "not update with patches" that its not same as "unsuported", but many other people like me still patched the older versions of php or whatever software in the net.. as example REDHAT linux still provides updates to the kernel 2.6 due performs better for many hardware due newer version assumed too modern hardware..

mckaygerhard commented 6 years ago

ah i forgot: its not necesary to said "it works" i posted the pull due the all other plugins depends of this .. so obviously i tested and works, so please merge the pull

and great work, those plugins are great, simple and maybe for someones, but great due does not alter the code of the osticket allowing updates from official (but very obtused) team

clonemeagain commented 6 years ago

The osticket crew does play their cards close.. 1.11 is "coming soon" for ages now! ;-)

  • must learn that new features promote new requirements, its the bussines of the MS virus

The new features in this case, make it easier/clearer what a piece of code is doing. Adds typehinting, enables more opcode caching and error-checking and IDE support.

Also, PHP is not a business, but an open source community, which you can join and vote on changes to the language, like this one: https://wiki.php.net/rfc/argument_unpacking#vote And you don't "have to learn them", the old code still works, I mean, several of the more anachronistic features get purged with each upgrade.. you will notice they are marked as deprecated for a few versions before being dumped. This is good I think. But they should be dumped into a lib for compatibility..

  • if no need of "new features" and everyting its working, dont touch it! works perfect why altered!

I completely agree!. But does it work perfectly? If it requires much more lines to define the outcome, is slower, with no typehinting support and therefore less compiler/opcode/IDE support.. and therefore is less optimizable by the interpreter and turns out to be much slower.. then no, I wouldn't say "it works perfectly". "it works" is not "it works perfectly".

  • php versions change not by "features", changes by "bussines" programers need to eat!

Most don't see PHP as a professional programming language to begin with.. attempts to make it so (like the change to 7) are resisted!.. must be very frustrating.

  • so the due i not a programer, i not think like this, i think "by needes" dont changing or complicating the things

Just to clarify, because I'm not sure I understand, you're claiming to not be a programmer, but are saying that complicating things is bad.. which is surely the philosophy of every programmer! :-)

  • evolution has one final target: extintion

I can't agree with this.

  • if all of this are too complicated to understang, read "making a genius" from László Polgár

I'm not sure what that has to do with complications or a discussion regarding changes to PHP versions and the features the team who release PHP have decided to add or remove.

mckaygerhard commented 6 years ago

hi @clonemeagain interesting ... let me adde some cents

do you noted that older software runs faster than newer.. that's all for all the resumend response my linux (kernel 2.6.8) debian sarge in my compaq c700 runs obviously faster lights rather thatn alpine (kernel 4.1), so all that you said depends of the "lot of features unnecesary or complexity" the MS lession learned was software must change to perform a change, forcing the change... by the obsolescense the newer versions runs "faster?", "better?".. please

great work with your plugins.. maybe a core plugin must be better but as is are good and its working

X-D

clonemeagain commented 6 years ago

I'm not being vague here, it's been repeatedly benchmarked, PHP7 is faster than PHP5. It consistently uses less RAM to get the same thing done in less time on the same hardware/vm.. it's therefore categorically better. I even find it better than HHVM, but that depends on your usage (we're moving to Laravel).

I get that it can be painful if you've used a lot of the older methods/frameworks, only to find that they are now deprecated into oblivion, we've been working for years to upgrade some custom intranet apps that were written for PHP4.. yup. Not 5.4, PHP4. That was brutal, but necessary.

One other thing that is often overlooked: programmer time! Which is a LOT more expensive than server time.

You'll find quite a few projects adding things that make it easier for contributors, because they want more contributors! It's the same with languages, they want devs to use their languages, so they make things easier for the people using the languages. IDE integration is useful as it speeds up dev's, opcode caching is useful as it speeds up prod, syntactic sugar (eg: splat!) is helpful because it makes things clearer and clear things are easier/faster to read for devs, typehinting speeds up opcodes, tests and IDE's, which means they can get up to speed faster and get productive faster, which means more dev output for the same time/money input!

mckaygerhard commented 6 years ago

yeah it's more faster but depends and its relative, php7 use some features from OS and hardware that performs more faster.. that's the reason, and due actually we usae modern hardware and in general modern OS's so in general for all php7 "performs better"

umm you have right, time programing its more expensive.. that's why i use Gambas for mayority of the projects.. more cheap, due everybody knows "visual basic"

i made my own tests, php7 in my Devuan 1.0 (jessie) with DELL poweredge for my intranet apps performs better as you said.. but in the Pentium fourt CPUs (fourt real cpu's in same motherboard at 233 MHz) php5 performs better..

was a interesting conversation..

mckaygerhard commented 6 years ago

hi @clonemeagain i let you this intersting article: https://spectreattack.com/spectre.pdf the very new GLOBAL CPU BUG based on "features over conservative speed" i'm using a K6-III and a 4 cpu atom on clusterized that are dont afecte by that global bug, but the os currently running does not support php 5.4+ a interesting very convenient example of new year!