Closed Bug 271272 Opened 16 years ago Closed 16 years ago

Use XML parsing on install.rdf instead of regex

Categories

(addons.mozilla.org Graveyard :: Developer Pages, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Bugzilla-alanjstrBugs, Assigned: ted)

References

()

Details

Attachments

(3 files, 5 obsolete files)

A technically correct install.rdf will be rejected because it does not match
exactly what the regular expression is looking for.  By using XML parsing with a
schema, we will be better at rejecting invalid install.rdfs (where things are
nested improperly) and valid install.rdfs (that place the namespace differently).
Blocks: 246945
Severity: normal → enhancement
Priority: -- → P5
Bulk Moving Developer Control Panel bugs to new component.
(Filter: massdevcpspam)
Component: Update → Developers
Product: mozilla.org → Update
Version: other → unspecified
Blocks: 271294
No longer blocks: 271294
Severity: enhancement → major
This caused bug 273550
A simple linebreak causes the GUID to be picked up wrong.
Patches welcome, until somebody takes this bug, I expect the bug settings not to
be changed.
Severity: major → enhancement
Priority: P5 → --
Target Milestone: --- → Future
Version: unspecified → 0.9
Is there any point in making a patch now?  Have you landed all of the mass
changes you're going to make?  I don't want to do work twice.
Assignee: nobody → Bugzilla-alanjstrBugs
This sucks.  My Extension Developer extension uses Mozilla's RDF serializer for
its install.rdf editor, which produces install.rdf files that UMO can't handle.
 Editing install.rdf by hand sucks.
Taking, patch coming right up.
Status: NEW → ASSIGNED
Attached file parse_install_manifest.php (obsolete) —
This uses the RDF class alanjstr linked above (which is a single php file).  I
stuck the RDF class in core/, and this file gets included from additem.php.
The rest.
Assignee: Bugzilla-alanjstrBugs → ted.mielczarek
Attachment #168376 - Flags: first-review?(psychoticwolf)
Attachment #168377 - Flags: first-review?(psychoticwolf)
Pike, can you sanity check my RDF parsing?
Comment on attachment 168377 [details] [diff] [review]
Changes to additem, parse install.rdf with an RDF parser instead of regexes.

>Index: additem.php
<...>
>@@ -297,21 +256,21 @@
<...>
>-
>+a

Really?
Ted -
As Pike pointed out, it looks like you fat-fingered something.  

Pike -
How's the rest of it look?
Comment on attachment 168376 [details]
parse_install_manifest.php

I have some issues with this, mostly l18y. Anyway, those are way-future.

See bug 274068 for the pending support of xml:lang within RDF.

On the code itself, I don't know RDF in php. But this seems to be a good
improvement. Hoping that the rdf lib is ok.

Oh, there may be issues where the Mozilla RDF parser accepts stuff that a
standards compliant parser wouldn't. But this shouldn't matter for RDF that
adheres to the extension manager grammar, as those validate by now.
Blocks: 268182
Please use escape_string() when you grab each item.
This doesn't look bad to me, at a glance anyway. Has this been tested to ensure
it doesn't break on existing items? (Including those that the site currently
breaks on, but more importantly, those it doesn't)

If it's been thouroughly tested (or can be in the next day or so), then I'm ok
with getting it reviewed and in for 1.0, otherwise it'll wait till 1.1.
Target Milestone: Future → 1.1
I've tested it on a variety of install.rdf files, both from my install.rdf
editor, and in the "standard" format accepted by UMO.  If you have any edge
cases you'd like me to test, I can certainly do that.  I'll get a cleaned up
patch attached tomorrow.
Nah, I'm mostly concerned with the mainstream cases, those generated by your
extension (which are correct) and the "standard" that we already handle
gracefully. :-)
Attached patch Updated patch (obsolete) — Splinter Review
Fixes the typo, fixes the one case where manifest data was used without
escape_string().

Axel, if you have i18n/l10n/whatever issues with this, please file other bugs
on me.	Thanks!
Attachment #168377 - Attachment is obsolete: true
Attachment #169753 - Flags: first-review?(psychoticwolf)
Moving big changes to 2.0.  
Target Milestone: 1.1 → 2.0
Attachment #168377 - Flags: first-review?(bugtrap)
Attachment #168376 - Flags: first-review?(bugtrap) → first-review?(Bugzilla-alanjstrBugs)
Attachment #169753 - Flags: first-review?(bugtrap) → first-review?(Bugzilla-alanjstrBugs)
Comment on attachment 169753 [details] [diff] [review]
Updated patch

Requesting R from CTho.  Don't forget to grab the php class, too.
Attachment #169753 - Flags: first-review?(Bugzilla-alanjstrBugs) → first-review?(cst)
Comment on attachment 168376 [details]
parse_install_manifest.php

What if someone wants to include descriptions in more than one language?  Does
this require http://www.mozilla.org/2004/em-rdf# to be available?  If so, what
if the site is down?

Why did you define $l = strlen(EM_NS); in the first function?
Comment on attachment 169753 [details] [diff] [review]
Updated patch

I'm assuming parse_install_manifest() does what it claims when I do this
review.

-      if ($release == $val[minversion]) {
$versioncheck[$key][minversion_valid] = "true"; }
-      if ($release == $val[maxversion]) {
$versioncheck[$key][maxversion_valid] = "true"; }
+      if ($release == $val["minVersion"]) {
$versioncheck[$key][minversion_valid] = "true"; }
+      if ($release == $val["maxVersion"]) {
$versioncheck[$key][maxversion_valid] = "true"; }

Why didn't you add quotes around minversion_valid / maxversion_valid? Any
reason you didn't capitalize min/maxversion_valid to be consistent with
minVersion and maxVersion?

 if ($versioncheck[errors]=="true") {
Should errors be in quotes?

-   $minappver = $manifestdata["targetapplication"]["$guid"]['minversion'];
-   $maxappver = $manifestdata["targetapplication"]["$guid"]['maxversion'];
+   $minappver = $manifestdata["targetApplication"]["$guid"]['minVersion'];
+   $maxappver = $manifestdata["targetApplication"]["$guid"]['maxVersion'];
Could you change the single quotes to double quotes as well?

+foreach ($manifestdata["targetApplication"] as $key=>$val) {
 //echo"$key -- $val[minversion] $val[maxversion]<br>\n";
Remove the comment, or change the capitalization so it will work when someone
uncomments it.
Attachment #169753 - Flags: first-review?(cst) → first-review-
Blocks: 278748
Attached patch parse_install_manifest.php (obsolete) — Splinter Review
Changed to support xml:lang for name and descriptions.	Also returns null on
parse errors now.
Attachment #168376 - Attachment is obsolete: true
Fixed per ctho's comments.  Also changed to fit with the xml:lang changes in
parse_install_manifest().
Attachment #169753 - Attachment is obsolete: true
Attachment #171558 - Flags: first-review?(cst)
Attachment #171557 - Flags: first-review?(Bugzilla-alanjstrBugs)
Comment on attachment 171557 [details] [diff] [review]
parse_install_manifest.php

Why aren't we ignoring iconURL, optionsURL, and aboutURL?

I'm glad you snagged updateURL because we'll need it for something else.

Why is there a commented code line
//$rdf->rdf_set_warning_handler("mf_warning_handler" );

Why do you define $l = strlen(EM_NS); inside of parse_install_manifest() ?
(In reply to comment #25)
> (From update of attachment 171557 [details] [diff] [review] [edit])
> Why aren't we ignoring iconURL, optionsURL, and aboutURL?

Good question.

> 
> I'm glad you snagged updateURL because we'll need it for something else.
> 
> Why is there a commented code line
> //$rdf->rdf_set_warning_handler("mf_warning_handler" );

Because I was using a warning handler but I removed it.  I can remove the comment.
 
> Why do you define $l = strlen(EM_NS); inside of parse_install_manifest() ?
 
Because I use the value twice, it saved me some typing and an extra calculation.
Attached file parse_install_manifest.php (obsolete) —
Once more, with feeling.
Attachment #171557 - Attachment is obsolete: true
Attachment #171561 - Flags: first-review?(Bugzilla-alanjstrBugs)
For real this time.
Attachment #171561 - Attachment is obsolete: true
Attachment #171562 - Flags: first-review?(Bugzilla-alanjstrBugs)
Attachment #171561 - Flags: first-review?(Bugzilla-alanjstrBugs) → first-review-
Attachment #171562 - Flags: first-review?(Bugzilla-alanjstrBugs) → first-review+
Attachment #171558 - Flags: first-review?(cst) → first-review+
Attachment #168376 - Flags: first-review?(Bugzilla-alanjstrBugs)
Attachment #171557 - Flags: first-review?(Bugzilla-alanjstrBugs)
Attached file class_rdf_parser.php
This is the RDF parsing class.	It should be added to core/
Checking in mozilla/webtools/update/core/class_rdf_parser.php;
/cvsroot/mozilla/webtools/update/core/class_rdf_parser.php,v  <-- 
class_rdf_parser.php
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/update/developers/parse_install_manifest.php
,v
done
Checking in mozilla/webtools/update/developers/parse_install_manifest.php;
/cvsroot/mozilla/webtools/update/developers/parse_install_manifest.php,v  <-- 
parse_install_manifest.php
initial revision: 1.1
done
Checking in mozilla/webtools/update/developers/additem.php;
/cvsroot/mozilla/webtools/update/developers/additem.php,v  <--  additem.php
new revision: 1.12; previous revision: 1.11
done
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Still needed on Branch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Branch:
Checking in mozilla/webtools/update/developers/additem.php;
/cvsroot/mozilla/webtools/update/developers/additem.php,v  <--  additem.php
new revision: 1.11.2.1; previous revision: 1.11
done
Checking in mozilla/webtools/update/developers/parse_install_manifest.php;
/cvsroot/mozilla/webtools/update/developers/parse_install_manifest.php,v  <--  p
arse_install_manifest.php
new revision: 1.1.2.1; previous revision: 1.1
done
Checking in mozilla/webtools/update/core/class_rdf_parser.php;
/cvsroot/mozilla/webtools/update/core/class_rdf_parser.php,v  <--  class_rdf_par
ser.php
new revision: 1.1.2.1; previous revision: 1.1
done
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Blocks: 274323
Blocks: 272651
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.