[
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.
