GNOME.org
 

Re: [xml] Potential wrong usage of xmlIsID() in tree.c

Kasimier Buchcik wrote:

Hi,

On Fri, 2006-02-03 at 11:34 -0500, Daniel Veillard wrote:

On Fri, Feb 03, 2006 at 05:24:30PM +0100, Kasimier Buchcik wrote:

Hi,

On Fri, 2006-02-03 at 11:13 -0500, Daniel Veillard wrote:

On Fri, Feb 03, 2006 at 03:52:46PM +0100, Kasimier Buchcik wrote:

Hi,

We have the following code in tree.c, xmlCopyPropInternal():


I think this should check if the newly created attribute is an ID in
the _destination_ doc.

  Hum, true, sounds there is somthing fishy there, though the itent is
to keep the IDness property, i.e. if you used #foo in th old doc to point
to that node, well #foo will still point to it in the new one.

Keep the IDness even if the target doc has a different or even no DTD?
If the attribute is an xml:id then yes, but otherwise I would say no.

  yeah, I know. But I expect if we change this we will break XInclude
because it uses that routine, and it carries IDness forward independantly
of the result DTD. So if you change this, please adjust how XInclude uses it to make sure XInclutde ain't broken as a result. 

Ah, I see, it's more complex than I thought.
You also can't forget user defined attributes, which currently are also not copied by XInclude. For example DOM level 3 allows IDness to be set or removed for any attribute when working on the tree without caring about or even requiring a DTD. These attributes are not being picked up as IDs during the copy.
A question on copying an attribute: Shouldn't the check that an attribute's parent element exist be removed? It should be possible to copy an xml:id or user defined attribute (not defined in DTD) without requiring a parent element correct? I would assume that since an xml:id can be created as an ID without an element that copying it would also create the copy as an ID. 
Just an observation: Currently, for branch-copy operations a DTD-lookup
is performed (together with a QName building string operation) for every
attribute (except for xml:id attributes).

There is more code in tree.c using xmlIsID() and I think some of
the calls should be substituted for the XML_ATTRIBUTE_ID-test.
E.g. in xmlSetProp(), xmlIsID() is called for an existent
attribute if it is about to be replaced by a new one.

Doesn't xmlIsID() have to be called _only_ if a newly created
attribute is added to the doc's tree? If it's already insideo

   Hum, it just test, and should not have side effect so I don't see why
we should be more restrictive in its use, plus it's part of the public API...


the doc's tree, then XML_ATTRIBUTE_ID should be
aready set - or am I missing something here?

  unclear,

Sorry, I was referring to the attr->atype == XML_ATTRIBUTE_ID check.
This marker is set on the attribute in xmlAddID(). We wouldn't have to
query through xmlIsID() if the marker is used to test IDness of attrs
already added to the doc's tree.
hum, right, that should work. I think thisd came from the fact that atype was added to xmlAttr but used to be only on xmlAttributes i.e. the 
declarations in the DTd not duplicated in the instances.


OK, I'll try to bugzilla for this next week (I need to go home now :-))

Checking the atype will also catch the user defined IDs mentioned before.
As far as the checks when unlinking an attribute, I was checking both atype and the result from xmlIsID(). This was to allow an attribute that was user defined as an ID or an xml:id attribute to remain an ID after being unlinked. This though looks like it might be a little broken for an xml:id attribute right now due to the additional checks in xmlIsID(). 

Rob
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]