This repository has been archived by the owner. It is now read-only.

Native Namespace Code Prep #1721

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
5 participants
@dongilbert
Contributor

dongilbert commented Nov 27, 2012

This PR adds in the root namespace backslash on calls to internal PHP and SPL classes. This will facilitate the platform moving to native PHP namespaces, whenever that may be.

More info can be found here: http://www.php.net/manual/en/language.namespaces.faq.php#language.namespaces.faq.globalclass

Also, I've replaced some explicit calls to class names with __CLASS__ in callables. For example, in JLoader when calling spl_autoload_register(). Also, where creating a new instance of the containing class, I've changed new JClassName to new static. I've not completed all instances of these yet.

This PR does not break B/C at all, as the whole codebase is already operating in the root/global namespace. But this will need to be done when we do move to namespaces, so I thought I could help that along.

No unit tests were harmed in the making of this pull request.

@eddieajau

This comment has been minimized.

Show comment Hide comment
@eddieajau

eddieajau Nov 27, 2012

Contributor

Thanks Don. Would you mind adding a page to the docs, probably namspacing.md under the Code Standards heading, which outlines what we need to be aware of when writing, and obviously reviewing, code?

It makes sense for the platform, but I think this could potentially give the CMS some grief with extension developers that want to retain support for PHP 5.2 on CMS 2.5 yet maintain a dual 2.5+3 version package. I don't have a problem with the change but it's probably worth giving the CMS dev list a heads up that we are talking about this.

Contributor

eddieajau commented Nov 27, 2012

Thanks Don. Would you mind adding a page to the docs, probably namspacing.md under the Code Standards heading, which outlines what we need to be aware of when writing, and obviously reviewing, code?

It makes sense for the platform, but I think this could potentially give the CMS some grief with extension developers that want to retain support for PHP 5.2 on CMS 2.5 yet maintain a dual 2.5+3 version package. I don't have a problem with the change but it's probably worth giving the CMS dev list a heads up that we are talking about this.

@dongilbert

This comment has been minimized.

Show comment Hide comment
@dongilbert

dongilbert Nov 27, 2012

Contributor

I was looking at adding to the documentation here - https://github.com/joomla/joomla-platform/blob/staging/docs/manual/en-US/coding-standards/chapters/php.md#class-definitions - but it's probably best to make it it's own page, since I'm sure the section will continue to grow. I'll get that added as well as inform the CMS dev list.

Contributor

dongilbert commented Nov 27, 2012

I was looking at adding to the documentation here - https://github.com/joomla/joomla-platform/blob/staging/docs/manual/en-US/coding-standards/chapters/php.md#class-definitions - but it's probably best to make it it's own page, since I'm sure the section will continue to grow. I'll get that added as well as inform the CMS dev list.

@eddieajau

This comment has been minimized.

Show comment Hide comment
@eddieajau

eddieajau Nov 27, 2012

Contributor

Thanks. I think it's better for it to be highly visible and I think it's easier to point to a single page on the topic when fielding questions (or criticism {cough}) about it.

Contributor

eddieajau commented Nov 27, 2012

Thanks. I think it's better for it to be highly visible and I think it's easier to point to a single page on the topic when fielding questions (or criticism {cough}) about it.

@dongilbert

This comment has been minimized.

Show comment Hide comment
@dongilbert

dongilbert Nov 28, 2012

Contributor

See documentation here - https://github.com/dongilbert/joomla-platform/blob/namespace_prep/docs/manual/en-US/coding-standards/chapters/namespacing.md

It's not completed yet, but it's what I could think of for now.

As for that compatibility layer section. We can take it or leave it, it's just my thoughts of what I would like to see happen, even if I maintain that layer myself for the 2 revisions after going fully namespaced.

Contributor

dongilbert commented Nov 28, 2012

See documentation here - https://github.com/dongilbert/joomla-platform/blob/namespace_prep/docs/manual/en-US/coding-standards/chapters/namespacing.md

It's not completed yet, but it's what I could think of for now.

As for that compatibility layer section. We can take it or leave it, it's just my thoughts of what I would like to see happen, even if I maintain that layer myself for the 2 revisions after going fully namespaced.

@eddieajau

This comment has been minimized.

Show comment Hide comment
@eddieajau

eddieajau Nov 28, 2012

Contributor

Compat. layer sounds good. I can get behind that if that's something we need (we should do some more clean up and shunting to the /legacy tree before we finalise it).

Contributor

eddieajau commented Nov 28, 2012

Compat. layer sounds good. I can get behind that if that's something we need (we should do some more clean up and shunting to the /legacy tree before we finalise it).

@dongilbert

This comment has been minimized.

Show comment Hide comment
@dongilbert

dongilbert Nov 28, 2012

Contributor

I agree - a lot of the compatibility needs could be buffered in the legacy tree, and then the remainder picked up by the proposed compatibility layer.

As you suggested in the mailing list, I think it's a great idea to hold off until 13.1 to merge this. Want to change the milestone?

Contributor

dongilbert commented Nov 28, 2012

I agree - a lot of the compatibility needs could be buffered in the legacy tree, and then the remainder picked up by the proposed compatibility layer.

As you suggested in the mailing list, I think it's a great idea to hold off until 13.1 to merge this. Want to change the milestone?

@eddieajau

This comment has been minimized.

Show comment Hide comment
@eddieajau

eddieajau Nov 28, 2012

Contributor

Bumped to 13.1.

Contributor

eddieajau commented Nov 28, 2012

Bumped to 13.1.

@nicksavov

This comment has been minimized.

Show comment Hide comment
@nicksavov

nicksavov Nov 28, 2012

Nice job, Don!

Nice job, Don!

@dongilbert

This comment has been minimized.

Show comment Hide comment
@dongilbert

dongilbert Nov 28, 2012

Contributor

Thanks - we pretty much put all our eggs in the J! basket at work - so if I want to use namespaces and all that has to offer, I need to make it happen.

Contributor

dongilbert commented Nov 28, 2012

Thanks - we pretty much put all our eggs in the J! basket at work - so if I want to use namespaces and all that has to offer, I need to make it happen.

@dongilbert

This comment has been minimized.

Show comment Hide comment
@dongilbert

dongilbert Nov 28, 2012

Contributor

Suggested way to handle the compatibility layer.

https://gist.github.com/4162595

Contributor

dongilbert commented Nov 28, 2012

Suggested way to handle the compatibility layer.

https://gist.github.com/4162595

@dongilbert

This comment has been minimized.

Show comment Hide comment
@dongilbert

dongilbert Dec 6, 2012

Contributor

Instead of __CLASS__, should we use $this when passing to a callable?

Contributor

dongilbert commented Dec 6, 2012

Instead of __CLASS__, should we use $this when passing to a callable?

@mbabker

View changes

libraries/joomla/data/data.php
@@ -17,7 +17,7 @@
* @subpackage Data
* @since 12.3
*/
-class JData implements JDataDumpable, IteratorAggregate, JsonSerializable, Countable
+class JData implements \JDataDumpable, \IteratorAggregate, \JsonSerializable, \Countable

This comment has been minimized.

Show comment Hide comment
@mbabker

mbabker Dec 24, 2012

Owner

JDataDumpable shouldn't have the slash here.

@mbabker

mbabker Dec 24, 2012

Owner

JDataDumpable shouldn't have the slash here.

@@ -519,7 +519,7 @@ public function unlockTables()
*/
public static function isSupported()
{
- return class_exists('PDO') && in_array('oci', PDO::getAvailableDrivers());
+ return class_exists('PDO') && in_array('oci', \PDO::getAvailableDrivers());

This comment has been minimized.

Show comment Hide comment
@realityking

realityking Jan 8, 2013

Contributor

Shouldn't this be class_exists('\PDO')?

@realityking

realityking Jan 8, 2013

Contributor

Shouldn't this be class_exists('\PDO')?

This comment has been minimized.

Show comment Hide comment
@dongilbert

dongilbert Jan 8, 2013

Contributor

Should be class_exists('\PDO'); <-- needs two \ since the \ is also the
escape character. You need to escape the escape.

I'm going to merge this in to my PlatformNamespace branch, since I'm going
through every file again as it is, and I know I missed some things before.
So - I should close this request. :)

On Mon, Jan 7, 2013 at 6:33 PM, Rouven Weßling notifications@github.comwrote:

In libraries/joomla/database/driver/oracle.php:

@@ -519,7 +519,7 @@ public function unlockTables()
*/
public static function isSupported()
{

  •   return class_exists('PDO') && in_array('oci', PDO::getAvailableDrivers());
    
  •   return class_exists('PDO') && in_array('oci', \PDO::getAvailableDrivers());
    

Shouldn't this be class_exists('\PDO')?


Reply to this email directly or view it on GitHubhttps://github.com/joomla/joomla-platform/pull/1721/files#r2572027.

@dongilbert

dongilbert Jan 8, 2013

Contributor

Should be class_exists('\PDO'); <-- needs two \ since the \ is also the
escape character. You need to escape the escape.

I'm going to merge this in to my PlatformNamespace branch, since I'm going
through every file again as it is, and I know I missed some things before.
So - I should close this request. :)

On Mon, Jan 7, 2013 at 6:33 PM, Rouven Weßling notifications@github.comwrote:

In libraries/joomla/database/driver/oracle.php:

@@ -519,7 +519,7 @@ public function unlockTables()
*/
public static function isSupported()
{

  •   return class_exists('PDO') && in_array('oci', PDO::getAvailableDrivers());
    
  •   return class_exists('PDO') && in_array('oci', \PDO::getAvailableDrivers());
    

Shouldn't this be class_exists('\PDO')?


Reply to this email directly or view it on GitHubhttps://github.com/joomla/joomla-platform/pull/1721/files#r2572027.

@dongilbert dongilbert closed this Jan 10, 2013

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.