Skip to content

Commit

Permalink
Improved hash generation for t:attr attribute (Fixes #23)
Browse files Browse the repository at this point in the history 
In certain edge case we can have spl_object_hash collisions, so lets use also some node data
  • Loading branch information
Asmir Mustafic committed Apr 22, 2015
1 parent 13f440b commit 7f34b8a
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/Goetas/Twital/Attribute/AttrAttribute.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ class AttrAttribute implements Attribute
{
public static function getVarname(\DOMNode $node)
{
return "__a9" . strtr(spl_object_hash($node), "-","_");
return "__a9" . strtr(md5($node->ownerDocument->saveXML($node)).spl_object_hash($node), "-","_");
}

public function visit(\DOMAttr $att, Compiler $context)
Expand Down

9 comments on commit 7f34b8a

@soukicz
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Collision is still possible, if both nodes have the same syntax. But I guess that in that case collision probably doesn't matter.

@goetas
Copy link
Owner

@goetas goetas commented on 7f34b8a Apr 25, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Soukiii I'm aware of it, but there is a PHP bug regarding the PHP's DOMDocument implementation (see https://bugs.php.net/bug.php?id=67459)

My solution is just a patch. In a ideal solution, since DOM objects are still alive, spl_object_hash should be enough.

I can not use your solution (uniq on similar random generators) because there are many plugins that relies on the non random value returned by AttrAttribute::getVarname (t:att-append, t:trans-attr at least).

@soukicz
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize my solution is wrong - it was just quick patch.

But this doesn't work if there is attr and attr-append on one node - they share variable but XML changes during proccesing.

<div t:attr="class='row'" t:attr-append=" class=' even'"></div>

@goetas
Copy link
Owner

@goetas goetas commented on 7f34b8a Apr 25, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh! Good point!

Re-opening the issue #23

@goetas
Copy link
Owner

@goetas goetas commented on 7f34b8a Apr 25, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a previous version of twital i tried to:

  • on a very first step, assign to each node a sequentially generated t:id attribute
  • using the t:id attribute (when available) instead of md5($node->ownerDocument->saveXML($node))
  • at very last step, remove all generated t:id attributes.

This solution is more "messy" but has can fix the problem exposed here by you.

What do you think about?

@soukicz
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it looks like that combination of attr and attr-append didn't work even before that change.

@soukicz
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is like what I tried to do. I tried to assign unique ID to special attribute but I didn't make it work. But my mistake was that I tried to use existing __attr__

@goetas
Copy link
Owner

@goetas goetas commented on 7f34b8a Apr 25, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Today i will try to create a PR on this way.

@goetas
Copy link
Owner

@goetas goetas commented on 7f34b8a Apr 25, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #24

Please sign in to comment.