Results of the CodeWorks code review
Joseph LeBlanc
Washington. They had a session devoted to code reviews run by
thePHP.cc (http://thephp.cc/en/site/who/). Several open-source
projects were available for the audience to choose from. Since Joomla
was one of the ones available, I requested it.
During the review, they pointed out a few things I think we can look
at, as well as several items I know are already in progress. These
guys are heavy proponents of unit testing, so many of their
suggestions were oriented around that. However, there were a few
others that I think we can fix before we get full unit testing in place.
We have a lot of functions throughout the core that are returning
references to objects, for instance:
public static function &getDocument()
However, there's no reason to return a reference in PHP unless you
have a specific reason to do so; they do not improve performance or
save memory: http://us2.php.net/manual/en/language.references.return.php
When I searched trunk for 'function &', I found 282 instances. We can
probably stand to eliminate most (if not all) of them, unless there's
a specific reason it's being done on such a widespread basis.
One of the other things they found peculiar was the DS constant we've
defined. According to them, there's no reason to do this as PHP
automatically converts / to \ when you're using Windows. They were
also puzzled as to why we had constants like JPATH_BASE without the
slash automatically appended at the end (although that probably loops
back to the DS constant issue).
Finally, require_once is a statement and not a function. I see a mix
in our codebase between require_once 'somefile.php' and
require_once('somefile.php');
They have some slides floating around with everything they found. They
were looking at 1.5.14.
I realize that some of this may seem picky, but since we're still pre-
beta on 1.6, I figure this might be a good chance to address some of
these issues. I'm willing to write patches unless there are good
reasons why we shouldn't change these things.
-Joe
Joseph LeBlanc
3rd party components at the moment. So we would probably hold off on
that.
-Joe
Mr Phil E. Taylor
past...
> --~--~---------~--~----~------------~-------~--~----~
> You received this message because you are subscribed to the Google Groups "Joomla! CMS Development" group.
> To post to this group, send email to joomla-...@googlegroups.com
> To unsubscribe from this group, send email to joomla-dev-cm...@googlegroups.com
> For more options, visit this group at http://groups.google.com/group/joomla-dev-cms?hl=en-GB
> -~----------~----~----~----~------~----~------~--~---
>
>
Ed Stafford
During the review, they pointed out a few things I think we can look
at, as well as several items I know are already in progress. These
guys are heavy proponents of unit testing, so many of their
suggestions were oriented around that. However, there were a few
others that I think we can fix before we get full unit testing in place.
I've just started writing unit tests for Joomla 1.6 as I didn't see any currently available. Is there already a suite of these for the existing codebase or would it be worth my continuing to write these? I've started at the libraries/joomla/base/ and working my way up. Mostly, I started this as I use Joomla a ton of places and wanted some practice with phpunit.
One of the other things they found peculiar was the DS constant we've
defined. According to them, there's no reason to do this as PHP
automatically converts / to \ when you're using Windows. They were
also puzzled as to why we had constants like JPATH_BASE without the
slash automatically appended at the end (although that probably loops
back to the DS constant issue).
Could you not remove DS from the core, but keep the constant defined for backwards compatibility?
Finally, require_once is a statement and not a function. I see a mix
in our codebase between require_once 'somefile.php' and
require_once('somefile.php');
The PHP reference describes it as require_once(), not require_once; so I would think just pick the reference standard and roll with it.
Ed Stafford
Niels Braczek
> Could you not remove DS from the core, but keep the constant defined for
> backwards compatibility?
+1
>> Finally, require_once is a statement and not a function. I see a mix
>> in our codebase between require_once 'somefile.php' and
>> require_once('somefile.php');
>
> The PHP reference describes it as require_once(), not require_once; so I
> would think just pick the reference standard and roll with it.
No need to repeat "bugs" from the reference. The correct syntax is
*without* parenthesis.
Another thing is, that require_once and include_once are
contraproductive, when using bytecaches. It is more appropriate to use
the autoload mechanism (spl_autoload, *not* __autoload!).
Regards,
Niels
--
| http://www.kolleg.de · Das Portal der Kollegs in Deutschland |
| http://www.bsds.de · BSDS Braczek Software- und DatenSysteme |
| Webdesign · Webhosting · e-Commerce · Joomla! Content Management |
------------------------------------------------------------------
Ian MacLennan
On Tue, Oct 6, 2009 at 12:50 PM, Joseph LeBlanc <con...@jlleblanc.com> wrote:During the review, they pointed out a few things I think we can look
at, as well as several items I know are already in progress. These
guys are heavy proponents of unit testing, so many of their
suggestions were oriented around that. However, there were a few
others that I think we can fix before we get full unit testing in place.
I've just started writing unit tests for Joomla 1.6 as I didn't see any currently available. Is there already a suite of these for the existing codebase or would it be worth my continuing to write these? I've started at the libraries/joomla/base/ and working my way up. Mostly, I started this as I use Joomla a ton of places and wanted some practice with phpunit.
There are some there, lots more to do.
Some more of the 1.5 ones could possibly be ported over as well.
Ian
Mark Dexter
http://joomlacode.org/svn/joomla/testing/trunk/1.6. Mark
Ian MacLennan
Ian
Ian MacLennan
Ian
Ed Stafford
On Tue, Oct 6, 2009 at 3:05 PM, Mark Dexter <dexter...@gmail.com> wrote:
Ed, you can check out the 1.6 Unit testing project here:
http://joomlacode.org/svn/joomla/testing/trunk/1.6. Mark
Ed
Ian MacLennan
Ian
Niels Braczek
> I'll have to clean up the tests I've written so far and submit them. To who
> or where should I send my patches/additions?
Unit tests have their own mailinglist, although it is quite unused yet:
joomla-unit...@googlegroups.com. I think it is a good place to
gather the unit test work and discussion.
Regards,
Niels
Louis Landry
Niels Braczek
> Assuming most, if not all, of these unit tests are going to be on framework
> libraries can we have that discussion on the framework list? If anyone is
> having trouble getting signed up on that list I will be happy to fix it.
So what is the reason for the unit test group? Will it be dropped?
That would be ok for me, but it does not make sense having an
infrastructure without using it.
Louis Landry
Andrew Eddie
Regards,
Andrew Eddie
http://www.theartofjoomla.com - the art of becoming a Joomla developer
2009/10/7 Louis Landry <louis....@joomla.org>:
Andrew Eddie
Thanks for the report. Very informative. "Most" (qualified) of the
require_once, include_once instances should be fixed in 1.6 already.
Dropping the DS (but leaving the constant) sounds great to me, as does
taking out the &'s (Phil is right, it's there for PHP 4 compat).
2009/10/7 Joseph LeBlanc <con...@jlleblanc.com>:
Christophe Demko
original object. In all other cases, do a classical return.
Joseph LeBlanc
In that case, I'll get to work on a patch for the remaining
require_once() and include_once()'es, then another one for references.
-Joe
Joseph LeBlanc
statements:
http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=18304
-Joe
Ercan Özkaya
On Oct 7, 7:57 pm, Joseph LeBlanc <cont...@jlleblanc.com> wrote:
> Patch ready for require_once, require, include, and include_once
> statements:
>
> -Joe
>
> On Oct 7, 2009, at 8:31 AM, Joseph LeBlanc wrote:
>
>
>
> > Hi Andrew,
>
> > In that case, I'll get to work on a patch for the remaining
> > require_once() and include_once()'es, then another one for references.
>
> > -Joe
>
> > On Oct 6, 2009, at 8:39 PM, Andrew Eddie wrote:
>
> >> Hi Joe
>
> >> Thanks for the report. Very informative. "Most" (qualified) of the
> >> require_once, include_once instances should be fixed in 1.6 already.
>
> >> Dropping the DS (but leaving the constant) sounds great to me, as
> >> does
> >> taking out the &'s (Phil is right, it's there for PHP 4 compat).
>
> >> Regards,
> >> Andrew Eddie
> >> developer
>
> >> 2009/10/7 Joseph LeBlanc <cont...@jlleblanc.com>:
Joseph LeBlanc
* It primarily deals with functions in libraries/joomla. There were some instances where code elsewhere was specifically depending on references; I skipped the ones I ran into issues with. Off the top of my head, the only one I can think of that did this was JNode, which was being used to generate the backend menu.
* A few fixes for component models are mixed in as well. From what I remember, these were causing issues immediately after I made specific patches, so I just went ahead and fixed them. If we want to deal with these separately as a patch for components, I can split this patch into libraries/joomla vs. other files.
* Documentation has changed to reflect the fact that we are now returning the objects, rather than references to the objects.
The patch is attached. I can add it to the tracker if necessary.
-Joe
Louis Landry
-Joe
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Joomla! CMS Development" group.
To post to this group, send email to joomla-...@googlegroups.com
To unsubscribe from this group, send email to joomla-dev-cm...@googlegroups.com
For more options, visit this group at http://groups.google.com/group/joomla-dev-cms?hl=en-GB
-~----------~----~----~----~------~----~------~--~---
On Oct 7, 2009, at 8:31 AM, Joseph LeBlanc wrote:
>
> --~--~---------~--~----~------------~-------~--~----~
> You received this message because you are subscribed to the Google Groups "Joomla! CMS Development" group.
> To post to this group, send email to joomla-...@googlegroups.com
> To unsubscribe from this group, send email to joomla-dev-cm...@googlegroups.com
> For more options, visit this group at http://groups.google.com/group/joomla-dev-cms?hl=en-GB
> -~----------~----~----~----~------~----~------~--~---
>
Louis Landry
Sorry, there was a multi-fail at work there. At the very least, the file got mangled when I posted it. I've now zipped and attached.-Joe
--
You received this message because you are subscribed to the Google Groups "Joomla! CMS Development" group.
To post to this group, send an email to joomla-...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/joomla-dev-cms?hl=en-GB.
--
You received this message because you are subscribed to the Google Groups "Joomla! CMS Development" group.
To post to this group, send an email to joomla-...@googlegroups.com.
To unsubscribe from this group, send email to joomla-dev-cm...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/joomla-dev-cms?hl=en-GB.