538428 - bugzilla.dtd is not valid

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Serge Gautherie (:sgautherie)]

Assuming that site is right...

{
Error (2)

Line	File name:	https://bugzilla.mozilla.org/bugzilla.dtd
72	Column:	31
 	Error:	Element type "delta_ts" must not be declared more than once.
 	Error Position:	<!ELEMENT delta_ts (#PCDATA)> 

Line	File name:	https://bugzilla.mozilla.org/bugzilla.dtd
86	Column:	12
 	Error:	Open quote is expected for attribute "type_id" associated with an element type "flag".
 	Error Position:	status CDATA #REQUIRED 
}

Comment 1

[Tanner M. Young [:tmyoung]]

The site is correct, these declarations prevent mozbot from receiving data, see Bug 559721.

Comment 2

[Tanner M. Young [:tmyoung]]

Attachment: Patch v 1.0

This removes the duplicate entry and adds #IMPLIED to the second one.

Comment 3

[Dave Miller [:justdave]]

(In reply to comment #2)
> This removes the duplicate entry and adds #IMPLIED to the second one.

And it also adds votes, was that intentional?

Comment 4

[Tanner M. Young [:tmyoung]]

Comment on attachment 440140 [details] [diff] [review]
Patch v 1.0

(In reply to comment #3)
> (In reply to comment #2)
> > This removes the duplicate entry and adds #IMPLIED to the second one.
> 
> And it also adds votes, was that intentional?

Nope...

Comment 5

[Tanner M. Young [:tmyoung]]

Attachment: Patch v. 1.1

Let's hope this one doesn't have votes...

Comment 6

[Dave Miller [:justdave]]

Comment on attachment 440141 [details] [diff] [review]
Patch v. 1.1

This looks good to me.

Comment 7

[Tanner M. Young [:tmyoung]]

Attachment: Patch for 3.6 branch

This is the same thing for the 3.6 branch.

Comment 8

[Frédéric Buclin]

We don't use this keyword, and the patch hasn't been approved yet.

Comment 9

[Frédéric Buclin]

Comment on attachment 440141 [details] [diff] [review]
Patch v. 1.1

>-<!ELEMENT delta_ts (#PCDATA)>

This one is used by the attachment element, while the other one is used by the bug element. Is it correct to remove it anyway? Also, this patch doesn't make bugzilla.dtd valid. There are much more errors than that.

Comment 10

[Frédéric Buclin]

Comment on attachment 440142 [details] [diff] [review]
Patch for 3.6 branch

Same comment as for the other patch.

Comment 11

[Tanner M. Young [:tmyoung]]

I don't know how much time I have groom the DTD from LpSolit's comments.  I will look at it again as soon as I can, but I know so little about DTDs.

If someone wants to take it over from me before I can fix it, please feel free.

My primary concern--and reason for the blocking-3.6.1 nomination--is that mozbot fails to work because of this, according to Bug 559721.

Comment 12

[Frédéric Buclin]

We won't block 3.6.1 on it.

Comment 13

[Tanner M. Young [:tmyoung]]

I am probably not going to have to work on this again in the few weeks/months, so I am setting back to NEW.  If I do have time, I will submit another patch.

Comment 14

[Tanner M. Young [:tmyoung]]

This is increasingly a problem as more mozbot users upgrade to newer versions of Bugzilla.

Comment 15

[David Lawrence [:dkl]]

Attachment: Patch to clean up the current bugzilla.dtd (v1)

I did some work to fix the issues reported and to also do some additional cleanup of the current dtd file. Please review.

dkl

Comment 17

[Frédéric Buclin]

Comment on attachment 527863 [details] [diff] [review]
Patch to clean up the current bugzilla.dtd (v1)

>+<!ELEMENT bug (bug_id, (alias?, creation_ts, short_desc, delta_ts+, reporter_accessible, 

There cannot be several delta_ts per bug.


>+<!ATTLIST reporter
>+          name CDATA #REQUIRED

qa_contact also has this attribute.


> <!ELEMENT group (#PCDATA)>
>+<!ELEMENT token (#PCDATA)>
> <!ATTLIST group
>           id CDATA #REQUIRED
>+>

Keep the group element and attlist grouped together.


>+<!ELEMENT long_desc (commentid?, attachid?, who, bug_when, work_time?, thetext)>

The commentid is mandatory.


> <!ATTLIST long_desc
>           encoding (base64) #IMPLIED
>           isprivate (0|1) #IMPLIED
>+>

long_desc hasn't the encoding attribute at all.


>+<!ELEMENT attachment (attachid, date, delta_ts, desc, filename?, type?, size?, attacher, token?, data?, flag*)>

filename, type and size are mandatory.


> <!ATTLIST attachment
>           isobsolete (0|1) #IMPLIED
>           ispatch (0|1) #IMPLIED
>           isprivate (0|1) #IMPLIED
>           isurl (0|1) #IMPLIED
> >

The first 3 attributes are required. isurl is gone in 4.1 and required in 4.0.


> <!ATTLIST flag
>+          type_id CDATA #IMPLIED

type_id is required.


>           setter CDATA #IMPLIED

setter is required too.


Otherwise looks good and passes my tests, except for the QA contact. Custom fields are not taken into account, but that a separate bug (bug 341809).

Comment 18

[Frédéric Buclin]

> <!ATTLIST long_desc
>           encoding (base64) #IMPLIED
>           isprivate (0|1) #IMPLIED
>+>

Also, isprivate is required.

Comment 19

[David Lawrence [:dkl]]

Attachment: Patch to clean up the current bugzilla.dtd (v2)

Thanks for the review. New revision addressing suggested changes.

dkl

Comment 20

[Frédéric Buclin]

Comment on attachment 528523 [details] [diff] [review]
Patch to clean up the current bugzilla.dtd (v2)

>=== modified file 'Bugzilla/Template.pm'
>-    # Now remove the encoding hacks
>+    # Now remove the encoding hacks and apply the replacements
>+    # in reverse order
>+    @things = reverse @things;

Oops, unrelated to this bug.



>=== modified file 'bugzilla.dtd'

>+<!ELEMENT attachment (attachid, date, delta_ts, desc, filename?, type?, size?, attacher, token?, data?, flag*)>

As I said in my previous review, filename, type and size are mandatory.


> <!ATTLIST attachment
>           isobsolete (0|1) #IMPLIED
>           ispatch (0|1) #IMPLIED
>           isprivate (0|1) #IMPLIED
> >

Also, these 3 attributes are also required.

Comment 21

[David Lawrence [:dkl]]

Attachment: Patch to clean up the current bugzilla.dtd (trunk) (v3)

Sorry about the extra file that got included. I have made the last changes and also removed isurl from the attachment definition for trunk. I will add another patch that adds it back for 4.0.

dkl

Comment 22

[David Lawrence [:dkl]]

Attachment: Patch to clean up the current bugzilla.dtd (4.0) (v1)

Version of patch for 4.0

dkl

Comment 23

[Frédéric Buclin]

Comment on attachment 528599 [details] [diff] [review]
Patch to clean up the current bugzilla.dtd (trunk) (v3)

Works fine, thanks. r=LpSolit

Comment 24

[Frédéric Buclin]

Comment on attachment 528601 [details] [diff] [review]
Patch to clean up the current bugzilla.dtd (4.0) (v1)

> <!ATTLIST attachment
>-          isobsolete (0|1) #IMPLIED
>-          ispatch (0|1) #IMPLIED
>-          isprivate (0|1) #IMPLIED
>+          isobsolete (0|1) #REQUIRED
>+          ispatch (0|1) #REQUIRED
>+          isprivate (0|1) #REQUIRED
>+          isurl (0|1) #REQUIRED
> >

I cannot apply this patch to the 4.0 branch. This is because isurl has been added manually, it looks like, but it's already there in 4.0.

Comment 25

[David Lawrence [:dkl]]

Attachment: Patch to clean up the current bugzilla.dtd (4.0) (v2)

Updated 4.0 patch that should apply now :)

Comment 26

[Frédéric Buclin]

Comment on attachment 528664 [details] [diff] [review]
Patch to clean up the current bugzilla.dtd (4.0) (v2)

>+    cc*, (estimated_time, remaining_time, actual_time, deadline)?, qa_contact?, 

The deadline can be undefined, even for users in the timetracking group. So it must be deadline?.


>+    token?, group*, flag*, long_desc*, attachment*)?)>

If the Voting extension is enabled, votes can appear right before token (and after qa_contact).


Please fix that on checkin, also on trunk. r=LpSolit

Comment 27

[Frédéric Buclin]

As we are close from the 4.0.1 release, I committed these patches myself.

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified bugzilla.dtd
Committed revision 7800.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.0/
modified bugzilla.dtd
Committed revision 7586.