SitePoint Sponsor

User Tag List

Results 1 to 11 of 11
  1. #1
    I solve practical problems. bronze trophy
    Michael Morris's Avatar
    Join Date
    Jan 2008
    Location
    Knoxville TN
    Posts
    2,011
    Mentioned
    56 Post(s)
    Tagged
    0 Thread(s)

    Namespace overriding.

    I've had some success with this, though I'm still a bit gun shy on it.

    Code php:
    namespace PNL;
     
    /**
     * Extend the root namespace function htmlentities to allow the first
     * argument to be an array or object.  If it is, recurse over it and apply
     * htmlentities to all members. KEYS WILL NOT CHANGE, so if they have html 
     * entities in them you still need to call htmlentities on the element.
     * 
     * @param mixed $mixed
     * @param unknown_type $flags
     * @param unknown_type $charset
     * @param unknown_type $double_encode
     */
    function htmlentities($mixed, $flags=null, $charset=null, $double_encode=null) {
    	if (is_array($mixed) || is_object($mixed)) {
    		foreach ($mixed as &$var) {
    			$var = htmlentities($var, $flags, $charset, $double_encode);
    		}
     
    		return $mixed;
    	} else {
    		return \htmlentities($mixed, $flags, $charset, $double_encode);
    	}
    }

    Thoughts (either on the overriding in general or the function itself)?

  2. #2
    Utopia, Inc. silver trophy
    ScallioXTX's Avatar
    Join Date
    Aug 2008
    Location
    The Netherlands
    Posts
    8,891
    Mentioned
    138 Post(s)
    Tagged
    2 Thread(s)
    1) It might be confusing, especially if your function is in long php files, that you're seemingly calling php's default htmlentities function which is actually your own. In this case it shouldn't really matter as the function is basically the same but extended, but if you drastically change a function people will have a hard time grasping why your code does what it does.

    2) Do you only have the PNL namespace? I ask because from what I read (I'm not too familiar with functions in namespaces), this function will only override the global function in the PNL namespace, not in subnamespaces of PNL, which makes it less interesting imo.

    3) I don't see the use of else in your code since the if part already returns something, which makes everything after the if body implicitly the else bodu, since it will only be executed when the if condition is not true.
    I would write it like this:

    Code php:
    namespace PNL;
     
    /**
     * Extend the root namespace function htmlentities to allow the first
     * argument to be an array or object.  If it is, recurse over it and apply
     * htmlentities to all members. KEYS WILL NOT CHANGE, so if they have html 
     * entities in them you still need to call htmlentities on the element.
     * 
     * @param mixed $mixed
     * @param unknown_type $flags
     * @param unknown_type $charset
     * @param unknown_type $double_encode
     */
    function htmlentities($mixed, $flags=null, $charset=null, $double_encode=null) {
    	if (is_array($mixed) || is_object($mixed)) {
    		foreach ($mixed as &$var) {
    			$var = htmlentities($var, $flags, $charset, $double_encode);
    		}
     
    		return $mixed;
    	}
            return \htmlentities($mixed, $flags, $charset, $double_encode);
    }

    That may just be a matter of taste of course, I don't know.
    Rémon - Hosting Advisor

    Minimal Bookmarks Tree
    My Google Chrome extension: browsing bookmarks made easy

  3. #3
    I solve practical problems. bronze trophy
    Michael Morris's Avatar
    Join Date
    Jan 2008
    Location
    Knoxville TN
    Posts
    2,011
    Mentioned
    56 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by ScallioXTX View Post
    1) It might be confusing, especially if your function is in long php files, that you're seemingly calling php's default htmlentities function which is actually your own. In this case it shouldn't really matter as the function is basically the same but extended, but if you drastically change a function people will have a hard time grasping why your code does what it does.
    I'm aware of that, but that's the reason this is an override - it does what the original does and some more. If it changed things, even slightly, I'd choose a different name.

    2) Do you only have the PNL namespace? I ask because from what I read (I'm not too familiar with functions in namespaces), this function will only override the global function in the PNL namespace, not in subnamespaces of PNL, which makes it less interesting imo.
    I typically use one namespace for the core framework and one for the project. The functions live in a 'definitions.php' file inside the namespace.

    Basically, I don't use the PHP recommendation for namespaces in my autoloader, which amounts to reflecting file structure in code structure, and will create major headaches for anyone using it, and I don't see it standing the test of time because it's ill thought out. Instead, classes live in files that share their name, and directories are ignored unless they have a .ns extension. The structure looks like this...

    Code:
    /php
      /PNL.ns
        Dispatcher.php
        definitions.php
        /arrays
          ReadOnlyArray.php
          StaticKeyArray.php
        /libraries
          TemplateLibrary.php
          TemplateLibraryFactory.php
    
      /TNT.ns
        Header.php
        definitions.php
    This allows me to organize my classes into directories however I feel like without facing coding ramifications. I use namespaces when I *need* to start a new naming scope.

    The autoloader is set up so that the first time it loads a class from a new namespace loads any definition files for that namespace. In the highly unlikely event a namespace function or constant must be referenced without loading any class from a namespace the definition load can be triggered with new \MyNameSpace\Definitions(); The autoloader will create an empty class to prevent an engine class. The unfortunate tradeoff is that "definitions" becomes unavailable as a class name in all namespaces.

    I would write it like this:

    Code php:
    namespace PNL;
     
    function htmlentities($mixed, $flags=null, $charset=null, $double_encode=null) {
    	if (is_array($mixed) || is_object($mixed)) {
    		foreach ($mixed as &$var) {
    			$var = htmlentities($var, $flags, $charset, $double_encode);
    		}
     
    		return $mixed;
    	}
            return \htmlentities($mixed, $flags, $charset, $double_encode);
    }

    That may just be a matter of taste of course, I don't know.
    EDIT - nevermind.

  4. #4
    SitePoint Wizard bronze trophy Jeff Mott's Avatar
    Join Date
    Jul 2009
    Posts
    1,148
    Mentioned
    14 Post(s)
    Tagged
    0 Thread(s)
    I'll grant you that this seems like a low risk intrusion into the global scope, but I still think the drawbacks outweigh the benefits. I think the most frequent drawback you'll encounter is ScallioXTX's #1: confusion. Other devs who try to read your code could be understandably confused when they see htmlentities behaving differently than the php.net docs say it should. Your custom escaper could instead be a method ($pnlTemplate->escape()) or even just a prefixed function (pnl_htmlentities()). I personally don't think either of those alternatives are so terrible to warrant monkey patching.
    "First make it work. Then make it better."

  5. #5
    SitePoint Wizard bronze trophy Jeff Mott's Avatar
    Join Date
    Jul 2009
    Posts
    1,148
    Mentioned
    14 Post(s)
    Tagged
    0 Thread(s)
    I've been dealing with a monkey patching issue today, and it made me think of this thread. (The issue I'm currently dealing with is in JavaScript, but the principle is the same.) The site I'm working on decided to "enhance" the global Date object. At the heart of my problem is the fact that it overrode the toString method. The enhanced toString method accepts a format parameter, and if no format parameter is provided, then it calls the original toString. Seems harmless... except another third-party script also decided to "enhance" the Date object's toString method in their own way, and the two aren't compatible.

    Any time you alter the global environment, you risk conflicts with other code that might also alter the global environment, or code that might depend on the global environment behaving exactly like it's expected to.

    I strongly recommend that you avoid at all costs any alterations to anything global.
    "First make it work. Then make it better."

  6. #6
    I solve practical problems. bronze trophy
    Michael Morris's Avatar
    Join Date
    Jan 2008
    Location
    Knoxville TN
    Posts
    2,011
    Mentioned
    56 Post(s)
    Tagged
    0 Thread(s)
    The \PNL namespace is not a global environment.

  7. #7
    SitePoint Wizard bronze trophy Jeff Mott's Avatar
    Join Date
    Jul 2009
    Posts
    1,148
    Mentioned
    14 Post(s)
    Tagged
    0 Thread(s)
    Ahh, my mistake. I thought you were overriding the global htmlentities. But you're right, that isn't what's happening. Feel free to ignore my posts.
    "First make it work. Then make it better."

  8. #8
    I solve practical problems. bronze trophy
    Michael Morris's Avatar
    Join Date
    Jan 2008
    Location
    Knoxville TN
    Posts
    2,011
    Mentioned
    56 Post(s)
    Tagged
    0 Thread(s)
    Your point isn't without merit, and I have decided to go ahead and attach that method to the Template class as #escape simply because I don't want to encourage html manipulation elsewhere. In PHP it isn't possible to force people to use a custom version of a function, but it could still be a problem for a maintenance programmer reading code within the namespace.

  9. #9
    SitePoint Wizard bronze trophy Jeff Mott's Avatar
    Join Date
    Jul 2009
    Posts
    1,148
    Mentioned
    14 Post(s)
    Tagged
    0 Thread(s)
    As a bonus, if you're interested, I like it when the behavior of a templating library is to escape by default. It makes it much harder to make a mistake and leave an XSS flaw somewhere. Here's a quick and simple way that could be done.

    PHP Code:
    class Escaper
    {
        private 
    $value;
        
        public function 
    __construct($value)
        {
            
    $this->value $value;
        }
        
        public function 
    __toString()
        {
            return 
    htmlspecialchars($this->valueENT_QUOTES);
        }
        
        public function 
    getRaw()
        {
            return 
    $this->value;
        }

    Escaper wraps some string value, and when an Escaper object is used in a string context, it returns the value escaped.

    PHP Code:
    $taintedContent '<b>Hello</b>, <i>World</i>';
    $safeContent = new Escaper($taintedContent);

    // Now we can use it in a string, and escaping happens automatically
    echo '<p>' $safeContent '</p>';

    // If we're sure we want the unescaped value, we can still get that
    echo '<p>' $safeContent->getRaw() . '</p>'
    "First make it work. Then make it better."

  10. #10
    I solve practical problems. bronze trophy
    Michael Morris's Avatar
    Join Date
    Jan 2008
    Location
    Knoxville TN
    Posts
    2,011
    Mentioned
    56 Post(s)
    Tagged
    0 Thread(s)
    My templates nest, that auto behavior would effectively lead to htmlentities(htmlentities()), which is counter productive.

  11. #11
    SitePoint Guru bronze trophy TomB's Avatar
    Join Date
    Oct 2005
    Location
    Milton Keynes, UK
    Posts
    988
    Mentioned
    9 Post(s)
    Tagged
    2 Thread(s)
    Ignoring the topic of overriding inbuilt functions, looping through an array/object and applying htmlentities to each field seems like poor separation of concerns. The escaping should be done at the output stage, not when (presumably) you're assigning variables to the template.

    The __toString() method shouldn't cause double escaping because it will only get called once, Jeff Mott's escaper function will work for arrays/objects with a little modification:

    PHP Code:

    class Escaper 

        private 
    $value
         
        public function 
    __construct($value
        { 
            
    $this->value $value
        } 
         
        public function 
    __toString() 
        { 
            return 
    htmlspecialchars($this->valueENT_QUOTES); 
        } 
         
        public function 
    getRaw() 
        { 
            return 
    $this->value
        } 

       public function 
    __get($name) {
           if (
    is_object($this->value)) return htmlentitites($this->value->$name);
           else if (
    is_array($this->value)) return htmlentitites($this->value[$name]);
       }

    PHP Code:

    <p>Your name is ' . <?=$user->firstname?> . ' ' . <?=$user->lastName;?></p>
    There's no way that can lead to double escaping.


Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •