PHP 5.3 Version

[clonemeagain]

Relates to issues #21, #22, #23 & #26

[clonemeagain]

Hmm, some of those commits have already been added to master.

[mckaygerhard]

umm the identation of your code change a lot!? ;-)

je je i just check this pull, i still get the problem, i try to study/read some of php 5.4 due seems i dont know enought..so i must read and learn now..

[clonemeagain]

Netbeans must be set to format before save.. lol

[clonemeagain]

What errors does this branch give on 5.3? I've only tested it on 7. Which isn't great.

[mckaygerhard]

sorry, i compared with diff and this does not have the discuted in the issue, now i'll will try later after read some about php 5.4 new features respect php 5.3, thanks for the hint

[mckaygerhard]

ahh the "memory hungry" netbeans.. yeah, with "auto complete docs" and lot fo "candy" stuff (yes i'm sarcastics ja ja ja)

[clonemeagain]

Changed the "php version" in Netbeans to 5.3 and it showed that last declaration in red. Handy! Goes properly red all over the show if I try dropping down to 5.2.

[mckaygerhard]

puff wow, netbeans have a built in engine check? well must compensate all the "requirementes" that last change ok but i talk about the usage of $this that since php5.4 can be used in anonymous functions and it refers to the current object, i'm pretty stupid or i cannot understand the comment you are posted in the issue #26

[clonemeagain]

You initially shared this error message:

Fatal error: Using $this when not in object context in /srv/intranet/elticket/include/plugins/attachment_preview/class.AttachmentPreviewPlugin.php on line 107

So, I checked what line 107 was, and it's the call_user_func() line that inserts things based on their extension. It doesn't actually send/use the $this object in another context, nor an anonymous function, so I think I got confused there. This PR had that section rewritten to avoid it (as detailed in #26, call_user_func call replaced with $this->{$method}($dom,$link);).

HOWEVER.. That wasn't the error! So, no wonder you were still seeing issues.

I looked into it, and the docs page illustrates when they changed: http://php.net/manual/en/functions.anonymous.php

version	message
7.1.0	Anonymous functions may not close over superglobals, $this, or any variable with the same name as a parameter.
5.4.0	Anonymous functions may use $this, as well as be declared statically.
5.3.0	Anonymous functions become available.

So, 5.3 can't use $this with an anonymous function, however that doesn't matter so much, because we aren't trying to (are we?), hopefully the only anonymous function in the plugin is the unFormatSize method, which uses functions to convert the size back .. that could be replaced with a switch/case structure actually.

Oop, just found another one.. the anonymous function defined in method log uses $this! damn. Removing that will entail changing private method log_after into a public static method, which is callable in the shutdown scope. Hmm.. We might be able to move it into the object's destructor, assuming that a destructor can make calls like that..

Oop, refer to $this->appended in static function appendHtml($html).. wow. Might have to make that an alias for add_arbitrary_html.. despite that not working the same way.

Ok.. found a few more. The bootstrap method makes good use of them.. and they make use of $this.. damn. Rewrote a few chunks, it tests OK on my 7.0 install..

[clonemeagain]

Ok.. maybe my checks would pass if I didn't try and optimize things too much:

There were 4 failures:
1) AttachmentPreviewPluginTest::testIsTicketsView with data set #2 ('https://tickets.dev/support/s...ex.php', true)
Failed asserting that true matches expected false.
/home/travis/build/clonemeagain/attachment_preview/tests/AttachmentPreviewPluginTest.php:106
2) AttachmentPreviewPluginTest::testIsTicketsView with data set #3 ('https://tickets.dev/support/s...ts.php', true)
Failed asserting that true matches expected false.
/home/travis/build/clonemeagain/attachment_preview/tests/AttachmentPreviewPluginTest.php:106
3) AttachmentPreviewPluginTest::testIsTicketsView with data set #6 ('http://crazylongdomainnametha...158279', true)
Failed asserting that true matches expected false.
/home/travis/build/clonemeagain/attachment_preview/tests/AttachmentPreviewPluginTest.php:106
4) AttachmentPreviewPluginTest::testIsTicketsViewPost with data set #0 ('https://tickets.dev/support/s...ts.php', array('open'), true)
Failed asserting that true matches expected false.```

[mckaygerhard]

errr, wel, most of do you said it's not easy to assimilate for me, i'm not so smart as you thing, ok let me test again and them:

hopefully the only anonymous function in the plugin is the unFormatSize method, which uses functions to convert the size back .. that could be replaced with a switch/case structure actually.

also:

Oop, just found another one.. the anonymous function defined in method log uses $this! damn.

now comes the "public" and "static" thing that i talking about.. so well we have now few checks passed lets try in my env..

[mckaygerhard]

uff checks passed let's try now

[clonemeagain]

Really simply: I got confused so went the wrong way trying to fix it. Then I thought about what you were saying and looked at it again, found plenty of anonymous functions using $this..

I've since refactored those out by adding a Singleton Factory to a constructor and moving the log failsafe to a destructor. This removes the $this use from the anonymous functions by linking to the single instance stored statically inside the object. It's a bit more complex than anticipated to explain, but really simply: I'm making sure only one copy of it ever runs, so certain assumptions can be made with that one copy.. like logs.

[clonemeagain]

Main concern now is the private call from a static context might fail as I'm calling it 'publically'.. but it might work as it's in the same class. We'll see I suppose.

Apparently it's fine! https://stackoverflow.com/a/3357878/276663 yay.

[mckaygerhard]

I've since refactored those out by adding a Singleton Factory to a constructor and moving the log failsafe to a destructor

the singleton factory, that performs that the same instance and no other are in use, so now i got more clarelly thanks for explanation.. i'm currently pulling and testing in my env (connection are very slow to the work)

[mckaygerhard]

well not errors but not render.. the anydesk are too slow currently to paste the errors, in php 5.4 its still woring but in php 5.3 does not render the complet page, i'll research tomorrow due the internet connection are very slow now..

well lets do that: if tomorrow still does nothign and the wor still are very hard, i'll will try to perform a conversation at my job to try setup php 5.4 in server

[clonemeagain]

I'd like to see the errors you're getting, because we are so close!

[mckaygerhard]

anydesk does not paste from remote to local, but said as:

arrrgg could see the desktop now.. try to paste from a shell web, nothign sorry please sorry but i lost connection, inclusivelly this page loads with difficulty now

[mckaygerhard]

theres something about the class:

fast-cgi error: shutdown handler for inline attachmentsrunning

was the last line, i set DEBUG to true

[mckaygerhard]

hi, fresh and active: puff i not see property error at all, well that's the only output i got in the error.log of the webserver: but the page does not render, in the firt try page renders only the header, if i hit "F5" not render any output.. now i'm see the process in the out, got a valit ticket and start5 to render, inspect the dom and got the attachment, the process stoping at the shutdown/startuing that confused to me

2018-01-04 22:42:46: (mod_fastcgi.c.2695) FastCGI-stderr: Matched /elticket/ as not ticket
2018-01-04 22:42:56: (mod_fastcgi.c.2695) FastCGI-stderr: Matched /elticket/logo.php as not ticket
2018-01-04 22:42:58: (mod_fastcgi.c.2695) FastCGI-stderr: Matched /elticket/login.php as not ticket
2018-01-04 22:43:00: (mod_fastcgi.c.2695) FastCGI-stderr: Matched /elticket/logo.php as not ticket
2018-01-04 22:43:01: (mod_fastcgi.c.2695) FastCGI-stderr: Matched /elticket/scp/ as not ticket
2018-01-04 22:43:03: (mod_fastcgi.c.2695) FastCGI-stderr: Matched /elticket/scp/logo.php?login as not ticket
2018-01-04 22:43:20: (mod_fastcgi.c.2695) FastCGI-stderr: Matched /elticket/scp/login.php as not ticket
2018-01-04 22:43:21: (mod_fastcgi.c.2695) FastCGI-stderr: Matched /elticket/scp/ as not ticket
2018-01-04 22:43:23: (mod_fastcgi.c.2695) FastCGI-stderr: Matched /elticket/scp/autocron.php as not ticket
2018-01-04 22:43:23: (mod_fastcgi.c.2695) FastCGI-stderr: Matched /elticket/scp/logo.php as not ticket
2018-01-04 22:43:25: (mod_fastcgi.c.2695) FastCGI-stderr: Matched /elticket/scp/ajax.php/config/scp as not ticket
2018-01-04 22:43:27: (mod_fastcgi.c.2695) FastCGI-stderr: Matched /elticket/scp/tickets.php?id=9&_pjax=%23pjax-container as ticket
2018-01-04 22:43:27: (mod_fastcgi.c.2695) FastCGI-stderr: AttachmentPreviewPlugin: Shutdown handler for inline attachments running
2018-01-05 09:03:45: (mod_fastcgi.c.2695) FastCGI-stderr: Matched /elticket/scp/tickets.php?id=9 as ticket
2018-01-05 09:03:45: (mod_fastcgi.c.2695) FastCGI-stderr: AttachmentPreviewPlugin: Shutdown handler for inline attachments running
2018-01-05 09:03:45: (mod_fastcgi.c.2695) FastCGI-stderr: AttachmentPreviewPlugin: Agent requested a tickets-view: Starting the attachments plugin.
2018-01-05 09:03:48: (mod_fastcgi.c.2695) FastCGI-stderr: Matched /elticket/scp/tickets.php?id=9 as ticket
2018-01-05 09:03:48: (mod_fastcgi.c.2695) FastCGI-stderr: AttachmentPreviewPlugin: Shutdown handler for inline attachments running
2018-01-05 09:03:48: (mod_fastcgi.c.2695) FastCGI-stderr: AttachmentPreviewPlugin: Agent requested a tickets-view: Starting the attachments plugin.

[mckaygerhard]

the second attemp when hit F5 are (noted that with php 5.4 and 7.0 works perfectly):

2018-01-05 09:30:37: (mod_fastcgi.c.2695) FastCGI-stderr: Matched /elticket/scp/admin.php as not ticket
2018-01-05 09:30:37: (mod_fastcgi.c.2695) FastCGI-stderr: Matched /elticket/scp/settings.php as not ticket
2018-01-05 09:30:38: (mod_fastcgi.c.2695) FastCGI-stderr: Matched /elticket/scp/autocron.php as not ticket
2018-01-05 09:30:38: (mod_fastcgi.c.2695) FastCGI-stderr: Matched /elticket/scp/logo.php as not ticket
2018-01-05 09:30:38: (mod_fastcgi.c.2695) FastCGI-stderr: Matched /elticket/scp/ajax.php/config/scp as not ticket
2018-01-05 09:30:40: (mod_fastcgi.c.2695) FastCGI-stderr: Matched /elticket/scp/index.php as ticket
2018-01-05 09:30:40: (mod_fastcgi.c.2695) FastCGI-stderr: AttachmentPreviewPlugin: Shutdown handler for inline attachments running
2018-01-05 09:30:44: (mod_fastcgi.c.2695) FastCGI-stderr: Matched /elticket/scp/index.php as ticket
2018-01-05 09:30:44: (mod_fastcgi.c.2695) FastCGI-stderr: AttachmentPreviewPlugin: Shutdown handler for inline attachments running
2018-01-05 09:31:13: (mod_fastcgi.c.2695) FastCGI-stderr: Matched /elticket/scp/index.php as ticket
2018-01-05 09:31:13: (mod_fastcgi.c.2695) FastCGI-stderr: AttachmentPreviewPlugin: Shutdown handler for inline attachments running

[clonemeagain]

I'm getting my own copy of 1.9 up and running on 5.3, believe the thread output to be different causing it to not match.

[mckaygerhard]

great @clonemeagain really really thanks for all of the helping. i'm currently deploying the webmail and the osticket at the job

[clonemeagain]

So, that latest commit brings compatibility with php 5.3 & php 7 for both 1.10.1 & 1.9.

It's still in DEBUG mode, so expect some extra logging while we're getting it working. Helps if you have duplicate log entries disabled in php.ini.. but runs ok for a dev version.

[clonemeagain]

Should change that other getElementsByTagName call into an XPath query.

[mckaygerhard]

excelent, testing, about the DEBUG mode, its a low profile work that a novice like me can help, so wheantime its good, really really thanks for that coding.. this plugin its the most importat of yours je je

[mckaygerhard]

hi @clonemeagain i tested, WORKING VERY GOOD IN PHP 5.3, also in 5.4, now i'll contribute (or i think i can do it) with a little change/problem that i have:

i tested in php 5..3, 5.4 and 5.6, works good, but i make a little change, do you know that irritating its the attachment open in same window, well, i changed that (due at the job something need to open large files) the change was open in popup window the files event in same, so i added a patch that enable the attachments to open in a previe window, but seems with this the plugin not work, so now if i use that patch plugin obviously does not work (due the DOM tree are not the xpected je je) so i think does not make sense but who knows, give some tips to re-implement that change with your plugin, i mean i made a if-else that detects that, i'm currently working on that but any other help are wwelcome, the pacht are that:

@@ -653,7 +653,7 @@ class ThreadEntry {
            if($attachment['size'])
                $size=sprintf('<em>(%s)</em>', Format::file_size($attachment['size']));

-            $str.=sprintf('<a class="Icon file no-pjax" href="%s" target="%s">%s</a>%s&nbsp;%s',
+            $str.=sprintf('<a class="Icon file no-pjax" href="%s" target="_blank" onClick="window.open(this.href, this.target, \'width=900,height=700\'); return false;">%s%s&nbsp;%s</a>',
                    $attachment['download_url'], $target, Format::htmlchars($attachment['name']), $size, $separator);
        }

[clonemeagain]

It's best not to patch core mate. You'll be reapplying them with every upgrade. That should fit in this plugin.

We already iterate through all the links via the DOM, so setting the target property of any or all of them is relatively easy, if awkward..

PJax will break, so you'll have to add the no-pjax class.

Something like:

$link->setAttribute('target', '_blank');
$link->setAttribute('class',$link->getAttribute('class'). ' no-pjax');

An admin toggle would be good, so I'd wrap that in an if to check the toggle and probably place it before the youtube check inside the foreach loop and inside the "is an attachment" strpos scope.

Syntax might be wrong, on mobile.

Not sure a popup would be popular.. but a new window/tab could be.

On Tue, 9 Jan. 2018, 05:54 PICCORO Lenz McKAY, notifications@github.com wrote:

hi @clonemeagain https://github.com/clonemeagain i tested, WORKING VERY GOOD IN PHP 5.3, also in 5.4, now i'll contribute (or i think i can do it) with a little change/problem that i have:

i tested in php 5..3, 5.4 and 5.6, works good, but i make a little change, do you know that irritating its the attachment open in same window, well, i changed that (due at the job something need to open large files) the change was open in popup window the files event in same, so i added a patch that enable the attachments to open in a previe window, but seems with this the plugin not work, so now if i use that patch plugin obviously does not work (due the DOM tree are not the xpected je je) so i think does not make sense but who knows, give some tips to re-implement that change with your plugin, i mean i made a if-else that detects that, i'm currently working on that but any other help are wwelcome, the pacht are that:

@@ -653,7 +653,7 @@ class ThreadEntry { if($attachment['size']) $size=sprintf('(%s)', Format::file_size($attachment['size']));

• $str.=sprintf('%s%s %s',+ $str.=sprintf('%s%s %s', $attachment['download_url'], $target, Format::htmlchars($attachment['name']), $size, $separator); }

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/clonemeagain/attachment_preview/pull/27#issuecomment-356059295, or mute the thread https://github.com/notifications/unsubscribe-auth/AE15j00movAKz1QQqsqmuKWpHBv1J4Ugks5tImRngaJpZM4RTzr- .

[mckaygerhard]

yeah, a new window its better (well the usage of the osticket its not preciselly in a mobile for staff), in any way its better rather than open in same window.. (all people are angry with that)

i have made some thing later i posted here...

[mckaygerhard]

hi @clonemeagain please merge this pull when the code will be beutifull for you, i tested and works very well as is.. (i can see the many times you have the loggin funtion to write je je, need a "if DEBUG" right)

i fixed, my pacht was wrong.. i'll corrected by reading your excelent comments, the link itterator when found valid get the extention but the "why reinventing the wheel" are not so "great",

seems the pathinfo due If the path has more than one extension, PATHINFO_EXTENSION returns only the last one so due no worry if i put a "onClick" or whatever, inside the "a" tag must be only dot's in the name of the file.. due my patch was include the part of the file size inside the link human readable will crash and not work.. so left the "size file" outside of the tag "a" as originally osticket do, solve the situation..

so a extra point in the a tag atrribute was the problem due the extra "%s" in my patch..:

@@ -653,7 +653,7 @@ class ThreadEntry {
            if($attachment['size'])
                $size=sprintf('<em>(%s)</em>', Format::file_size($attachment['size']));

-            $str.=sprintf('<a class="Icon file no-pjax" href="%s" target="%s">%s</a>%s&nbsp;%s',
+            $str.=sprintf('<a class="Icon file no-pjax" href="%s" target="_blank" onClick="window.open(this.href, this.target, \'width=900,height=700\'); return false;">%s</a>%s&nbsp;%s',
                    $attachment['download_url'], $target, Format::htmlchars($attachment['name']), $size, $separator);
        }

[clonemeagain]

Ah dude, class.thread.php is a core file, not part of this plugin.

Added a way that works on mine, with admin toggle.

[clonemeagain]

I'm thinking that would have been so much easier in jQuery

[mckaygerhard]

umm interesting.. works like a charm in php 5.4 and also in 5.3.. but the preview still opens in same window, sound borring but in all the methods the perform the previews (add_) we could introduce same so .. its my suggestion.. would be better

i put my core patch in my fork due are very little, but you have right ..

[mckaygerhard]

hey, jquery are more easy as coding standars, but force the osticket plugin to support limited range of browsers to most modern.. with the code as is, all my remote staff people can use it without problems..

as exampe differents versions of midory, or the broken older stpudi ie 7 and 8 too..

well i confess to you, some countries do not have "so modern" hardware to so then use "so modern" os's... why change if are working.. do you what i means?

[clonemeagain]

Well, in jQuery, it would be

$('a.file').prop('target','_blank');

Yup. That's it. lol.

[mckaygerhard]

yeah .. very easy.. also the headaches when "x brower yet now its not compatible anymore, upgrade... !" etc etc, i passed to that problems.. belive me! its a pain..

hey merge this brand in master.. its working! i want to post a new patch to you .. but i dont want you see it before this compatibility brand are merged!

[clonemeagain]

I'm not sure I follow, the plugin uses jQuery to function. So does osticket. jQuery itself is incredibly compatible mate.

[mckaygerhard]

yeah.. you have right.. i speak too quickly, in fact its non-sense in some degree, now talking about the pull, i just tested at the job, very usefully, i think its the best made by you, maybe better (in functionallity) rather than autoclose plugin

[mckaygerhard]

"all checks have passed" XD

[clonemeagain]

Noice. Rewrote the install commands into a script that checks the tag out from the osticket repo instead of decompressing a gzipped tar, seems more likely to keep working.

Current weirdness is for PHP5.4 (not installed by default, deprecated etc) and OT 1.9, not working 100%, but enough to pass the unittests:

0.30s$ phpunit
PHP Warning:  include(/home/travis/build/clonemeagain/attachment_preview/tests/plugin.php): failed to open stream: No such file or directory in /home/travis/build/clonemeagain/osticket/include/class.plugin.php on line 280
Warning: include(/home/travis/build/clonemeagain/attachment_preview/tests/plugin.php): failed to open stream: No such file or directory in /home/travis/build/clonemeagain/osticket/include/class.plugin.php on line 280
PHP Warning:  include(): Failed opening '/home/travis/build/clonemeagain/attachment_preview/tests/plugin.php' for inclusion (include_path='.:/home/travis/.phpenv/versions/5.4.45/share/pear') in /home/travis/build/clonemeagain/osticket/include/class.plugin.php on line 280
Warning: include(): Failed opening '/home/travis/build/clonemeagain/attachment_preview/tests/plugin.php' for inclusion (include_path='.:/home/travis/.phpenv/versions/5.4.45/share/pear') in /home/travis/build/clonemeagain/osticket/include/class.plugin.php on line 280
PHP Warning:  array_merge(): Argument #2 is not an array in /home/travis/build/clonemeagain/osticket/include/class.plugin.php on line 280
Warning: array_merge(): Argument #2 is not an array in /home/travis/build/clonemeagain/osticket/include/class.plugin.php on line 280
PHPUnit 4.8.35 by Sebastian Bergmann and contributors.
........................
Time: 206 ms, Memory: 15.00MB
OK (24 tests, 25 assertions)
The command "phpunit" exited with 0.

Specifically excluded 7.1/1.9 tests from the build matrix, because they aren't compatible.

I'm thinking there is some 'legacy' plugin code in the 1.9 codebase, check the comment before that line in class.plugin.php: // plugin.php is require to return an array of informaiton about // the plugin. lol

[mckaygerhard]

that's becose was a future implementation not implemented.. the api changes a lot to try to improve less information unuseless.. i'm personally happy with the development model of osticket.. its clarelly that their changes are following the "money changes" event the comunity contribution.. some years ago i remenber so much contributions good made that today are complety loss...

[clonemeagain]

Well, maybe it's time we forked it with all the good PR's and simply rebase them whenever they release.