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

[Kasimier Buchcik <K Buchcik 4commerce de>]

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():
> > > >
> > > > if ((target!= NULL) && (cur!= NULL) &&
> > > >         (target->doc != NULL) && (cur->doc != NULL) &&
> > > >         (cur->doc->ids != NULL) && (cur->parent != NULL)) {
> > > >         if (xmlIsID(cur->doc, cur->parent, cur)) {
> > > >   [... add the id to the target doc ...]
> > > > }
> > > >
> > > > This queries if the current attr is an ID in the _source_ doc via
> > > > xmlIsID().
> > > >
> > > > 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.

> > > > 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 :-))

Regards,

Kasimier