[jira] Commented: (SHINDIG-103) New PHP Shindig gadget server code

["Kevin Brown (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/SHINDIG-103?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12574302#action_12574302 ] 

Kevin Brown commented on SHINDIG-103:
-------------------------------------

Hi Chris,

So far this looks pretty good. Looking at the GadgetSpecParser implementation reminded me of all the things I actually love about PHP, and looking at the stuff you had to do to deal with the lack of namespacing reminded me of the things I hate :) 

Does anyone know if there's a condensed patch format that would make this patch easier to look at? Specifically, I want something that will make all of the file removal lines not include every single line from the original file. The patch would probably be about half as big if it weren't for that.

I've got a few comments. 

- Don't copy the features / javascript directories. This will make maintenance and compatibility between the php and java implementations very difficult. For all intents and purposes, features/ should be considered a separate project, and anything implemented in features/ should be language-neutral. This is the only change that I'd consider a requirement for submitting this patch -- everything else is just suggestion.

- In general, documentation is lacking. I'm not sure I would have been able to figure out what was going on here if I weren't already very familiar with the Java side of things. This is a common failing in many parts of the Java code as well, but I'm working to rectify that very soon. Did you have plans to add this later?

- I wouldn't go out of my way to emulate Java too much in PHP. The programming paradigms are so fundamentally different that what makes good sense in Java is frequently a bad idea in PHP, and vice versa. Beyond that, the current Java code isn't really ideal from an architectural standpoint anyway, and significant refactoring is coming down the pipeline relatively soon. Specifically, I'd avoid emulating GadgetSpec/ GadgetView / Gadget -- they're just plain badly implemented, and we don't have an excuse for that other than time constraints. Additionally, our XML parsers are confined by Java's horrible XML handling capabilities, whereas PHP has the brilliant SimpleXML extension. Do what makes sense in PHP rather than trying to maintain a class for class port. We have all kinds of specifications and standards to ensure compatibility :) I wouldn't go rewriting anything now, of course, but don't feel obligated to try to work the same way as the Java code -- focus on complying with the spec and everything else will follow.

Some additional suggestions (feel free to use or ignore these as you like -- the way I see it, whoever writes the code dictates design choices):

- define() is horribly slow for no good reason in PHP, and it prevents many optimizations from happening when apc or zend cache are enabled -- you can probably get some significant performance improvements by replacing your current configuration mechanism with a single array, and then just include this file in your scripts that need configuration (even better -- wrap it in a configuration class).

- include_once is also horribly slow for no good reason (noticing a theme here?) when using APC. If you can avoid it, try just using include -- and don't use the parens.  include is a language construct, not a function call.

- You don't need null checks when you're already doing empty() tests. empty() will always return true for null values.

- equals methods are generally unnecessary in PHP, since the default == operator does a value comparison anyway, and it's recursive. The one caveat would be if you have infinite recursion in your references. See http://www.php.net/manual/en/language.oop5.object-comparison.php

- Have you considered using __autoload to ensure that dependencies are pulled in exactly once? There's a small performance hit for using it, but it makes maintenance quite a bit easier. 

- Your use of a single main entry point is definitely the right way to go. One huge benefit that this has (aside from obvious architectural ones) is that you could actually just include all classes at the top level. This allows for huge performance gains when APC or other opcode caches are used.

- SimpleXML allows you to access attributes as $element['foo'], so you don't have to call $element->attributes['foo']. This might simplify some of your logic in the spec parser.

- You can replace Substitutions::substituteType with the following: 
   return str_replace(array_keys($this->substitutions[type]), array_values($this->substitutions[$type])); 
byte-by-byte parsing of the body makes sense in Java, but it yields poor performance in PHP. You'd want to store the keys as __<TYPE>_<key>__ to make this work, of course.

- You can get the unmodified request headers in PHP using getallheaders whenever you're running inside of apache. I suppose there are people using PHP without apache, but I think other assumptions you make require it. You could also probably just make requests to the "open" proxy use mod_proxy as well.

> New PHP Shindig gadget server code
> ----------------------------------
>
>                 Key: SHINDIG-103
>                 URL: https://issues.apache.org/jira/browse/SHINDIG-103
>             Project: Shindig
>          Issue Type: Improvement
>          Components: Gadgets Server - PHP
>            Reporter: Chris Chabot
>            Assignee: Brian McCallister
>         Attachments: php-shindig.patch
>
>
> This patch completely replaces the old code in the php directory of the shindig project, and replaces it with a port of the java shindig code.
> The java code used is from a checkout from February the 14th, this because keeping track of the changes while porting and testing would have gotten in the way of completing the first release in a timely fashion.
> It implements the structure and workings from the java server and includes:
> - Gadget server
> - Proxy service
> - JS service
> The maIn differences from the Java branch are:
> - Different caching engine (abstracted base class, sample file implementation included)
> - Different gadget rendering work flow (no multithreading in PHP)
> - No enum's or name spaced classes in php, so their top level classes now
> - Locale class to emulate the Java's Locale class
> - Somewhat servlet like event handling to stay close to the Java's structure
> Missing is the RPC feature, some custom speedups to get it closer to the Java's speed (it's about 40% slower right now, but that gap might be fixed later on), and a better logging / error reporting structure, it now only has a basic DEBUG flag in the config, which if set, causes it to dump debug backtrace's on errors.
> It's also missing Caja support, since that is Java only at the moment. Maybe somewhere in the future we'll have a command line version available that we can use.
> It also contains a copy of the features directory structure (so that it doesn't break on updates from the Java side of things, changes will have to be implemented before you can copy over the new version) and a copy of the sample container (minus the Caja options), so that people can test it directly.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.