DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 [EMAIL PROTECTED] changed: What|Removed |Added Status|NEW |RESOLVED Resolution||DUPLICATE --- Additional Comments From [EMAIL PROTECTED] 2007-04-23 08:09 --- This bug (and others) are addressed in PATCH http://issues.apache.org/bugzilla/show_bug.cgi?id=41831 *** This bug has been marked as a duplicate of 41831 *** -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 [EMAIL PROTECTED] changed: What|Removed |Added Status|ASSIGNED|NEW --- Additional Comments From [EMAIL PROTECTED] 2007-02-27 00:36 --- (In reply to comment #32) (In reply to comment #31) Created an attachment (id=19578) -- (http://issues.apache.org/bugzilla/attachment.cgi?id=19578action=view) [edit] [edit] main patch file Patch finally applied, with some modifications (rev. 507539). Thanks Adrian! I removed the test if (!dir.exists()) in FopFactory.getBaseURLfromConfig because it doesn't work when the property is a URL. Perhaps you didn't know, but Hmmm, by doing this I broke the test_fontbase_bad.xconf testcase, which no longer throws a ConfigurationException. I could swear I ran the testcases before applying the patch :-\. Re-opening the bug, will handle that later. Vincent -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 --- Additional Comments From [EMAIL PROTECTED] 2007-02-14 06:28 --- (In reply to comment #31) Created an attachment (id=19578) -- (http://issues.apache.org/bugzilla/attachment.cgi?id=19578action=view) [edit] main patch file Patch finally applied, with some modifications (rev. 507539). Thanks Adrian! I removed the test if (!dir.exists()) in FopFactory.getBaseURLfromConfig because it doesn't work when the property is a URL. Perhaps you didn't know, but this is actually possible, for user-friendliness, to specify base and font-base either as URLs or as directory paths. The purpose of this method is to convert a path into a URL if necessary. If the given value actually is a path and does correspond to an existing directory, everything's fine. Otherwise, we assume that this is a URL whose resolution will be attempted later on, and possibly fail. The patch is still useful and fixes bug #40120. Bug #40288 keeps open, because the full URL isn't displayed in the error message yet. If you wish you're welcome to continue your work on validation. For example you may want to add testcases with URLs instead of paths as parameters for base and font-base. Thanks, Vincent -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 --- Additional Comments From [EMAIL PROTECTED] 2007-02-13 01:46 --- (In reply to comment #28) Created an attachment (id=19576) -- (http://issues.apache.org/bugzilla/attachment.cgi?id=19576action=view) [edit] main patch file I'v had problems again applying the patch: something went wrong with FontSetup, looks like the patch wants to remove this file entirely, which I don't think should be the case. Also, why did you change the DEFAULT_STRICT_FO_VALIDATION constant into DEFAULT_STRICT_VALIDATION? I think it would be clearer to leave it as is. Thanks, Vincent -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 --- Additional Comments From [EMAIL PROTECTED] 2007-02-13 03:16 --- (In reply to comment #29) (In reply to comment #28) Created an attachment (id=19576) -- (http://issues.apache.org/bugzilla/attachment.cgi?id=19576action=view) [edit] [edit] main patch file I'v had problems again applying the patch: something went wrong with FontSetup, looks like the patch wants to remove this file entirely, which I don't think should be the case. Hmm.. not sure quite why that happened. Also, why did you change the DEFAULT_STRICT_FO_VALIDATION constant into DEFAULT_STRICT_VALIDATION? I think it would be clearer to leave it as is. Have done. -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 [EMAIL PROTECTED] changed: What|Removed |Added Attachment #19576|0 |1 is obsolete|| --- Additional Comments From [EMAIL PROTECTED] 2007-02-13 03:17 --- Created an attachment (id=19578) -- (http://issues.apache.org/bugzilla/attachment.cgi?id=19578action=view) main patch file -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 --- Additional Comments From [EMAIL PROTECTED] 2007-02-12 02:10 --- (In reply to comment #23) Adrian, thanks for doing this! I've looked at the patch and have a few comments myself: - I'd suggest to rename strict-configuration to validate-configuration (just a personal preference). The configuration will still be validated regardless of the setting of this variable. Its just how the error is handled that makes the difference. If strict-configuration is true then FOP will immediately throw an exception and processing will terminate. If strict-configuration is set to false then FOP will log the error and attempt to continue parsing the configuration (if possible/meaningful). - the name of the variable strictFO in FopFactory.configure(Configuration) seems wrong. There's nothing FO specific there. Furthermore, some if (strictFO) should actually be if (strictConfig), right? I think you may have been looking at an older patch. The variable strictValidation is as before and the new variable is called strictUserConfigValidation. Adrian, would you please install the CheckStyle plug-in in your IDE? There are a few nits about the Java style in your patch. Checkstyle will help you find them. Get well quickly, Adrian! After a weekend in bed am feeling much better today thanks. I had installed checksytle but not enabled it! ;-( I will recreate the patch this morning. All the best, Adrian. -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 [EMAIL PROTECTED] changed: What|Removed |Added Attachment #19561|0 |1 is obsolete|| --- Additional Comments From [EMAIL PROTECTED] 2007-02-12 03:41 --- Created an attachment (id=19574) -- (http://issues.apache.org/bugzilla/attachment.cgi?id=19574action=view) main patch file This *should* be the last patch file I upload for this bug :-). -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 --- Additional Comments From [EMAIL PROTECTED] 2007-02-12 08:01 --- (In reply to comment #24) (In reply to comment #23) Adrian, thanks for doing this! I've looked at the patch and have a few comments myself: - I'd suggest to rename strict-configuration to validate-configuration (just a personal preference). The configuration will still be validated regardless of the setting of this variable. Its just how the error is handled that makes the difference. If strict-configuration is true then FOP will immediately throw an exception and processing will terminate. If strict-configuration is set to false then FOP will log the error and attempt to continue parsing the configuration (if possible/meaningful). - the name of the variable strictFO in FopFactory.configure(Configuration) seems wrong. There's nothing FO specific there. Furthermore, some if (strictFO) should actually be if (strictConfig), right? I think you may have been looking at an older patch. The variable strictValidation is as before and the new variable is called strictUserConfigValidation. No, Jeremias is right actually. There are 'if (strictFO)' statements to test if an exception has to be thrown. That should be 'if (validateUserConfigStrictly())'. I should have better looked. Also, in the new code a HyphenationTreeResolver is no longer created and assigned to the hyphResolver field. That should inevitably lead to a NullPointerException later, however the hyphenation junit tests pass. Strange. Are you sure of what you're doing? One other thing: when getting the default-page-settings parameter it's not necessary to test if pageConfig is null; it will never be. See the javadoc of getChild. Adrian, would you please install the CheckStyle plug-in in your IDE? There are a few nits about the Java style in your patch. Checkstyle will help you find them. Something checkstyle doesn't seem to be catching: we usually don't put spaces inside brackets: if (test) instead of if ( test ). Get well quickly, Adrian! After a weekend in bed am feeling much better today thanks. I had installed checksytle but not enabled it! ;-( I will recreate the patch this morning. Hope you feel better now! Sorry, I'll ask you to create yet another patch, but that should really be the last one! Thanks, Vincent -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 --- Additional Comments From [EMAIL PROTECTED] 2007-02-12 09:02 --- Hi Vincent, Thanks very much for taking the time to look closely at my patch. (In reply to comment #26) (In reply to comment #24) (In reply to comment #23) Adrian, thanks for doing this! I've looked at the patch and have a few comments myself: - I'd suggest to rename strict-configuration to validate-configuration (just a personal preference). The configuration will still be validated regardless of the setting of this variable. Its just how the error is handled that makes the difference. If strict-configuration is true then FOP will immediately throw an exception and processing will terminate. If strict-configuration is set to false then FOP will log the error and attempt to continue parsing the configuration (if possible/meaningful). - the name of the variable strictFO in FopFactory.configure(Configuration) seems wrong. There's nothing FO specific there. Furthermore, some if (strictFO) should actually be if (strictConfig), right? I think you may have been looking at an older patch. The variable strictValidation is as before and the new variable is called strictUserConfigValidation. No, Jeremias is right actually. There are 'if (strictFO)' statements to test if an exception has to be thrown. That should be 'if (validateUserConfigStrictly())'. I should have better looked. Darn it.. I didn't think Jeremias was refering to the configure method. I missed this, thanks. Also, in the new code a HyphenationTreeResolver is no longer created and assigned to the hyphResolver field. That should inevitably lead to a NullPointerException later, however the hyphenation junit tests pass. Strange. Are you sure of what you're doing? Thanks for checking this, I made a mistake refactoring here :-(. One other thing: when getting the default-page-settings parameter it's not necessary to test if pageConfig is null; it will never be. See the javadoc of getChild. I've read the javadoc and have simplified/removed this unnecessary check now. Adrian, would you please install the CheckStyle plug-in in your IDE? There are a few nits about the Java style in your patch. Checkstyle will help you find them. Something checkstyle doesn't seem to be catching: we usually don't put spaces inside brackets: if (test) instead of if ( test ). Yes I have noticed this recently and believe all the code I've added adheres to this now. Get well quickly, Adrian! After a weekend in bed am feeling much better today thanks. I had installed checksytle but not enabled it! ;-( I will recreate the patch this morning. Hope you feel better now! Sorry, I'll ask you to create yet another patch, but that should really be the last one! Its ok, I am learning all the bumps for fop patch submissions - hopefully should be smoother for you guys next time :-). Kind regards, Adrian. -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
Re: DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
Hi Adrian, Adrian Cumiskey a écrit : Vincent Hennebert wrote: Yes, the patch doesn't seem to break anything. We could even go a bit further: the cfg parameter is no longer used in the FOUserAgent.configure() method, it might be removed. Also, would the FopFactory.getUserConfig() method not be public, I would even remove it, and move the logic that is in FOUserAgent.getUserRendererConfig into FopFactory. Is that getUserConfig() method supposed to have any utility to embedding applications? I would answer no, but what do embedding specialists think about it? Vincent I had considered all these points when developing the patch, and I whole heartedly agree with them, they are all good ones :-).. but I wanted to try and be as minimal as possible in my refactoring - at the end of the day this should just be a fairly simple, straight forward patch after all. Making these sort of changes makes committer checking more difficult and time consuming. If you are happy to review my patch file with the above changes I am *more than happy* to make them :-). Don't be afraid of refactoring, if it brings significant simplifications. To help the reviewing work, I suggest you to do the following: - don't do unrelated changes in the same patch. Just address one problem at a time; a big change addressing one problem will be simpler to track than several unrelated little changes; - give a detailed changelog along with your patch: that will allow us to check your proposed changes are ok instead of inferring them; - if you move files, tell us, so that we can do svn move instead of deleting the file from the old location and creating a new one, as would be the case by blindly applying the patch. That will allow to keep the history of changes. Going further along this train of thought.. IMO, in an ideal world all this config stuff should be abstracted in a separate class such as FopConfig - so code dependencies on the avalon Configurable interface would be minimised and configuration would be centrally managed. Interesting. I'm not a specialist of the config area, so I hope someone who has more insight in this area will be able to give comments. If you want to give it a try, please do. Anything that goes towards simplifying the code base is a good thing, IMO. Thanks, Vincent
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 --- Additional Comments From [EMAIL PROTECTED] 2007-02-09 01:33 --- (In reply to comment #19) (From update of attachment 19493 [edit]) this is now contain in the main patch file I have compilation errors when applying this patch: PDFRenderer, PSRenderer and XMLRenderer now expect the FontSetup.buildFontListFromConfiguration method to have 2 arguments, which is not the case. It seems you forgot to include the modifications of FontSetup in your latest patch. Also, did you manage to reproduce the SAXParseException error? Thanks, Vincent -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 [EMAIL PROTECTED] changed: What|Removed |Added Attachment #19548|0 |1 is obsolete|| --- Additional Comments From [EMAIL PROTECTED] 2007-02-09 04:24 --- Created an attachment (id=19561) -- (http://issues.apache.org/bugzilla/attachment.cgi?id=19561action=view) main patch file Hi Vincent, I have the flu today but checked my email earlier this morning and noticed your reply. I have attached a new patch.. I did leave FontSetup out.. it is one of the files I am working on for the font directory functionality. I haven't experienced any SAXException problems, all the config unit tests passed fine. I will try and investigate this when I am feeling a little better. Wishing you a nice weekend, Adrian. -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
Re: DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
On 09.02.2007 10:21:23 Vincent Hennebert wrote: snip/ Going further along this train of thought.. IMO, in an ideal world all this config stuff should be abstracted in a separate class such as FopConfig - so code dependencies on the avalon Configurable interface would be minimised and configuration would be centrally managed. Interesting. I'm not a specialist of the config area, so I hope someone who has more insight in this area will be able to give comments. If you want to give it a try, please do. Anything that goes towards simplifying the code base is a good thing, IMO. A central config class is probably not a bad idea. Some people have asked how fonts could be configured programmatically. That's not so easy at the moment. Something like that could simplify it with the nice side-effect to make FopFactory and FOUserAgent a little smaller. OTOH, the use of Avalon Framework allows for a big amount of flexibility. The renderers can be configured from the central configuration without knowing how the particular configuration layout for the renderer looks like (Configurable interface). This is handy if you use custom renderers, for example. If the configuration were limited to a central object structure it would limit extensibility quite a bit. Maybe we need something in between: A strongly coupled base config for FOP core and loosly coupled configuration for the plug-ins (renderers, XML handlers etc.). Not sure how best to solve this dilemma. Flexibility vs. user-friendlyness. Hmm. Thinking about configuration I end up each time being a little unhappy how fonts are configured: per renderer. Especially now that we've decided not to pursue the FOrayFont road at this time, we have to think about the direction we want to take. I'm still thinking around a font source concept (discussed in 2003): FOP provides a number of font sources and each renderer can choose fonts from the set of font sources it supports. Font sources would be: Java2D subsystem, Type 1 fonts, TrueType/OpenType fonts, PCL fonts, AFP fonts etc. Something like that would allow a proper one-time configuration of all fonts and lets the renderers use whatever they need. But that's another story/thread. Jeremias Maerki
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 --- Additional Comments From [EMAIL PROTECTED] 2007-02-09 07:10 --- (In reply to comment #16) I yet encountered this kind of error: it occurs during xsd validation if there is extra spaces or line brak before the XML declaration. HTH, lightbulb/ Indeed, this is probably due to extra ^M at the end of lines in the new files. When converting the patch to the unix file format the problem no longer appeared. Thanks for the hint Pascal! Adrian, please create your patch from a unix box next time. Wait, just kidding :-P -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 --- Additional Comments From [EMAIL PROTECTED] 2007-02-08 02:28 --- (In reply to comment #10) I had a short look at the patch, and I have the following comments: Do not change the 0.93 documentation but the trunk documentation: src/documentation/content/xdocs/trunk/configuration.xml, etc. Oops, yes I missed that. I will update the patch file to make changes to the trunk xdocs and not 0.93. TargetResolution is a property of FOUserAgent because it may easily vary with each run. It is not a good idea if you move properties around. They are in FopFactory or in FOUserAgent for a reason. See http://xmlgraphics.apache.org/fop/trunk/embedding.html. There is no real change here. TargetResolution is still a property of FOUserAgent, it is just configured from FopFactory and propagated to the FOUserAgent. As before, the value of this property in FOUserAgent can be programmatically set at anytime during rendering. Although I did not (and will not) have enough time to look at the patch in detail, I have the feeling that it addresses more than user configuration validation. It does some refactoring. That requires careful inspection. It may look like quite a large patch, but there really isn't very much refactoring, just stricter value checking which addresses issues raised in bugs 40288 and 40120. -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 [EMAIL PROTECTED] changed: What|Removed |Added Attachment #19540|0 |1 is obsolete|| --- Additional Comments From [EMAIL PROTECTED] 2007-02-08 02:42 --- Created an attachment (id=19545) -- (http://issues.apache.org/bugzilla/attachment.cgi?id=19545action=view) main patch file Who ever commits this patch will need to remember to update fop/status.xml with something resembling the following (remember to fill in the dev attribute in the action tag). Index: status.xml === --- status.xml (revision 504579) +++ status.xml (working copy) @@ -28,6 +28,9 @@ changes release version=FOP Trunk + action context=Code dev=?? type=fix fixes-bug=40120,40288 due-to=Adrian Cumiskey +Strict FOP user configuration checking. + /action action context=Code dev=JM type=add Support for GIF images in RTF output (RTF handler, only. Does not affect the RTF library.) /action -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 --- Additional Comments From [EMAIL PROTECTED] 2007-02-08 07:02 --- (In reply to comment #12) Created an attachment (id=19545) -- (http://issues.apache.org/bugzilla/attachment.cgi?id=19545action=view) [edit] main patch file I have another comment: this would be better to put the test config files in a subdirectory of test/ --typically, test/conf. Also, the tests fail with a SAXParseException: The processing instruction target matching [xX][mM][lL] is not allowed. Can aynyone else try and tell me if the problem also occurs? Finally, I got a small error applying your patch. I could recover easily, but next time be sure to create it from scratch and against the latest svn revision. Thanks, Vincent -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
Re: DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 --- Additional Comments From [EMAIL PROTECTED] 2007-02-08 02:28 --- (In reply to comment #10) I had a short look at the patch, and I have the following comments: Do not change the 0.93 documentation but the trunk documentation: src/documentation/content/xdocs/trunk/configuration.xml, etc. Oops, yes I missed that. I will update the patch file to make changes to the trunk xdocs and not 0.93. TargetResolution is a property of FOUserAgent because it may easily vary with each run. It is not a good idea if you move properties around. They are in FopFactory or in FOUserAgent for a reason. See http://xmlgraphics.apache.org/fop/trunk/embedding.html. There is no real change here. TargetResolution is still a property of FOUserAgent, it is just configured from FopFactory and propagated to the FOUserAgent. As before, the value of this property in FOUserAgent can be programmatically set at anytime during rendering. Yes, the patch doesn't seem to break anything. We could even go a bit further: the cfg parameter is no longer used in the FOUserAgent.configure() method, it might be removed. Also, would the FopFactory.getUserConfig() method not be public, I would even remove it, and move the logic that is in FOUserAgent.getUserRendererConfig into FopFactory. Is that getUserConfig() method supposed to have any utility to embedding applications? I would answer no, but what do embedding specialists think about it? Vincent
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 --- Additional Comments From [EMAIL PROTECTED] 2007-02-08 07:20 --- (In reply to comment #13) (In reply to comment #12) Created an attachment (id=19545) -- (http://issues.apache.org/bugzilla/attachment.cgi?id=19545action=view) [edit] [edit] main patch file I have another comment: this would be better to put the test config files in a subdirectory of test/ --typically, test/conf. The reason I didn't do this is because of concerns raised about this patch containing too much refactoring. But I am in agreement that it makes sense for the config files to live separately. So if nobody objects I will provide them in a separate zip attachment and move/remove the existing test/test.xconf to test/config/test.xconfig. Also, the tests fail with a SAXParseException: The processing instruction target matching [xX][mM][lL] is not allowed. Can aynyone else try and tell me if the problem also occurs? Do you have the stack trace for this? Finally, I got a small error applying your patch. I could recover easily, but next time be sure to create it from scratch and against the latest svn revision. Thanks, Vincent Sorry about this, I will try to be mindful of this in the future. Ideally I would do this everytime, but there have been a number of revisions to this patch and preparing a new patch from scratch every half day is not a good use of time. Its not always practically quick and easy to do as I am in parallel cracking on with other FOP development work. regards, Adrian. -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
Re: DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
Vincent Hennebert wrote: Yes, the patch doesn't seem to break anything. We could even go a bit further: the cfg parameter is no longer used in the FOUserAgent.configure() method, it might be removed. Also, would the FopFactory.getUserConfig() method not be public, I would even remove it, and move the logic that is in FOUserAgent.getUserRendererConfig into FopFactory. Is that getUserConfig() method supposed to have any utility to embedding applications? I would answer no, but what do embedding specialists think about it? Vincent I had considered all these points when developing the patch, and I whole heartedly agree with them, they are all good ones :-).. but I wanted to try and be as minimal as possible in my refactoring - at the end of the day this should just be a fairly simple, straight forward patch after all. Making these sort of changes makes committer checking more difficult and time consuming. If you are happy to review my patch file with the above changes I am *more than happy* to make them :-). Going further along this train of thought.. IMO, in an ideal world all this config stuff should be abstracted in a separate class such as FopConfig - so code dependencies on the avalon Configurable interface would be minimised and configuration would be centrally managed. regards, Adrian.
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 --- Additional Comments From [EMAIL PROTECTED] 2007-02-08 07:40 --- (In reply to comment #14) (In reply to comment #13) (In reply to comment #12) Created an attachment (id=19545) -- (http://issues.apache.org/bugzilla/attachment.cgi?id=19545action=view) [edit] [edit] [edit] main patch file I have another comment: this would be better to put the test config files in a subdirectory of test/ --typically, test/conf. The reason I didn't do this is because of concerns raised about this patch containing too much refactoring. But I am in agreement that it makes sense for the config files to live separately. So if nobody objects I will provide them in a separate zip attachment and move/remove the existing test/test.xconf to test/config/test.xconfig. Well, the existing test.xconf might be used for other purposes. AFAIU its purpose its not to test that the handling of user config files is correct, but only to configure other tests. So I would leave it as is and put in the conf/ subdirectory only files meant as tests for the config handling code. Also, the tests fail with a SAXParseException: The processing instruction target matching [xX][mM][lL] is not allowed. Can aynyone else try and tell me if the problem also occurs? Do you have the stack trace for this? Of course, sorry, here it is: at org.apache.fop.config.BaseUserConfigTestCase.testUserConfig(BaseUserConfigTestCase.java:6 8) Same error message for all of the testcases. It's a bit weird, I'm not even sure to understand it. Finally, I got a small error applying your patch. I could recover easily, but next time be sure to create it from scratch and against the latest svn revision. Thanks, Vincent Sorry about this, I will try to be mindful of this in the future. Ideally I would do this everytime, but there have been a number of revisions to this patch and preparing a new patch from scratch every half day is not a good use of time. Its not always practically quick and easy to do as I am in parallel cracking on with other FOP development work. I can understand. You may want to work on several copies of the Trunk for each different task. That way it will be very easy to generate new patches. Thanks, Vincent -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 [EMAIL PROTECTED] changed: What|Removed |Added Attachment #19545|0 |1 is obsolete|| --- Additional Comments From [EMAIL PROTECTED] 2007-02-08 08:19 --- Created an attachment (id=19548) -- (http://issues.apache.org/bugzilla/attachment.cgi?id=19548action=view) main patch file Contains suggested ammendments from Vincent. -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 [EMAIL PROTECTED] changed: What|Removed |Added Attachment #19514|0 |1 is obsolete|| --- Additional Comments From [EMAIL PROTECTED] 2007-02-08 08:21 --- (From update of attachment 19514) this is now included in the main patch file -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 [EMAIL PROTECTED] changed: What|Removed |Added Attachment #19493|0 |1 is obsolete|| --- Additional Comments From [EMAIL PROTECTED] 2007-02-08 08:27 --- (From update of attachment 19493) this is now contain in the main patch file -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
Re: DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
Hi Adrian, Sorry for the delay. I've had a look at your patch. First of all, thanks for taking care of this. Strong validation of user config files is a great step towards user-friendliness. Thanks also for thinking of documentation and testcases! I have a few comments, some of which are open for discussion with other devs: - we may discuss about the interest of a separate/yet another parameter for validating the config file. I can understand both points of vue and, actually, I don't really care of which is chosen. That said, I think validation of the config file should be enabled by default (as strict FO validation, IIC) because this will be helpful to newbies. Power users will be clever enough to disable it if they want to. So I suggest you to change the default value to true in your patch. - the changes in src/documentation/content/xdocs/0.93/fonts.xml are a bit inexact. That's ok, you're probably still learning. But: - the examples you added have different noticeable behaviours: the first one gets the font's metrics through a beforehand generated XML file and won't embed the font in the PDF file, because it doesn't have access to the font file; the second ones computes the metrics on the fly and is able to embed the font in the PDF file. As those differences are not trivial and are explained further on the page I would leave the example as is. - if both files are specified the metrics-url won't take the precedence, at least not exactly. The metrics defined in this file will override the metrics in the font (if the user modified the file by hand after generating it), but the embed-url file will always be used to load and embed the font. - AFAIK, kerning is available for PS and PDF outputs. - but as I said, thanks for updating the documentation! - I ask for other devs' opinions, but in strict user-config validation mode I would not also log an error when an exception is subsequently thrown. Reporting the problem just once is enough IMO. - In apps.FopFactory: - AFAIU the validatesStrictly() method is not related to user configuration, as the javadoc seems to imply - setUserConfig doesn't throw SAXException or IOException, but FOPException If no other fop-dev have objections to my comments above, I'd like to ask you to post a new patch with the suggested corrections. It should be ready to be applied then. Thanks, Vincent --- Additional Comments From [EMAIL PROTECTED] 2007-02-02 03:55 --- Created an attachment (id=19497) -- (http://issues.apache.org/bugzilla/attachment.cgi?id=19497action=view) main patch file (In reply to comment #3) The patch is fairly large and seems to address more than only validation of the configuration file. Can you give a summary? The patch seems quite large because I have added a new suite of configuration unit tests for the fix (these are contained in the attached zip file). A summary of the changes is given below. I am against this exception being thrown if I do not use the font arial at all: [ERROR] FOP - Exception org.apache.avalon.framework.configuration.ConfigurationException: Failed to resolve font metric-url 'arial.xml'org.apache.avalon.framework.configuration.ConfigurationException: Failed to resolve font metric-url 'arial.xml' In order for this to be the case, the user configuration would have to be (re-)read/parsed at the runtime of the rendering operation in order that it would be able to know if a particular font is being used in the given FO file. This would be an inefficient way of doing things and would require some structural changes. I am also against combining strict validation of the FO file and the user configuration; they are two different things. I would appreciate a separate option 'validate-configuration', which checks every entry in the configuration file. I did raise this as a question on forum and I remember you responded to it. http://www.nabble.com/Bad-FOP-configurations-tf3064300.html#a8522251 Chris Bowditch suggested that it be combined with strict-validation. I can see your argument for this and this was my original idea but I can also see Chris' argument too. On balance I have decided to go with your suggestion and make it separate a separate configuration option so as to not cause any confusion. I have introduced a new configuration variable called strict-configuration. I haven't called it validate-configuration because validation should always occur, its how the resulting error is handled that is the important thing (see Andreas' comments from the thread). If strict-configuration == false then FOP will simply log the error and continue to parse the user configuration. If strict-configuration == true then FOP will immediately raise an exception and will not continue trying to configure itself. I have attached a new patch file containing these changes (this
Re: DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
Hi Vincent,
First of all, many thanks for your comments/feedback.
Vincent Hennebert wrote:
Hi Adrian,
Sorry for the delay. I've had a look at your patch. First of all, thanks
for taking care of this. Strong validation of user config files is
a great step towards user-friendliness. Thanks also for thinking of
documentation and testcases!
I have a few comments, some of which are open for discussion with other
devs:
- we may discuss about the interest of a separate/yet another parameter
for validating the config file. I can understand both points of vue
and, actually, I don't really care of which is chosen. That said,
I think validation of the config file should be enabled by default (as
strict FO validation, IIC) because this will be helpful to newbies.
Power users will be clever enough to disable it if they want to. So
I suggest you to change the default value to true in your patch.
My thinking behind initially switching this option off was to not break
any existing configurations, but you are right its probably best to help
the newbies and not worry too much about initially upsetting some of the
power users. I take your point and will make this change.
- the changes in src/documentation/content/xdocs/0.93/fonts.xml are
a bit inexact. That's ok, you're probably still learning. But:
- the examples you added have different noticeable behaviours: the
first one gets the font's metrics through a beforehand generated XML
file and won't embed the font in the PDF file, because it doesn't
have access to the font file; the second ones computes the metrics
on the fly and is able to embed the font in the PDF file. As those
differences are not trivial and are explained further on the page
I would leave the example as is.
I will revert this change.
- if both files are specified the metrics-url won't take the
precedence, at least not exactly. The metrics defined in this file
will override the metrics in the font (if the user modified the file
by hand after generating it), but the embed-url file will always be
used to load and embed the font.
I'm not sure this is the case (but maybe it should be..). All the
config fonts are loaded on demand by the LazyFont encapsulation. If you
look at org.apache.fop.fonts.LazyFont.load(), the metrics url does take
precedence over the embed url (see conditional tests on metricsFileName
and fontEmbedPath).
private void load(boolean fail) {
if (!isMetricsLoaded) {
try {
if (metricsFileName != null) {
/[EMAIL PROTECTED] Possible thread problem here */
FontReader reader = null;
if (resolver != null) {
Source source = resolver.resolve(metricsFileName);
if (source == null) {
String err = Cannot load font: failed to
create Source from metrics file
+ metricsFileName;
if (fail) {
throw new RuntimeException(err);
} else {
log.error(err);
}
return;
}
InputStream in = null;
if (source instanceof StreamSource) {
in = ((StreamSource) source).getInputStream();
}
if (in == null source.getSystemId() != null) {
in = new
java.net.URL(source.getSystemId()).openStream();
}
if (in == null) {
String err = Cannot load font: failed to
create InputStream from
+ Source for metrics file +
metricsFileName;
if (fail) {
throw new RuntimeException(err);
} else {
log.error(err);
}
return;
}
InputSource src = new InputSource(in);
src.setSystemId(source.getSystemId());
reader = new FontReader(src);
} else {
reader
= new FontReader(new InputSource(new
URL(metricsFileName).openStream()));
}
reader.setKerningEnabled(useKerning);
reader.setFontEmbedPath(fontEmbedPath);
reader.setResolver(resolver);
realFont = reader.getFont();
} else {
if (fontEmbedPath == null) {
throw new RuntimeException(Cannot load font.
No font URIs available.);
}
Re: DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
Hi Adrian, Adrian Cumiskey a écrit : snip/ - we may discuss about the interest of a separate/yet another parameter for validating the config file. I can understand both points of vue and, actually, I don't really care of which is chosen. That said, I think validation of the config file should be enabled by default (as strict FO validation, IIC) because this will be helpful to newbies. Power users will be clever enough to disable it if they want to. So I suggest you to change the default value to true in your patch. My thinking behind initially switching this option off was to not break any existing configurations, but you are right its probably best to help the newbies and not worry too much about initially upsetting some of the power users. I take your point and will make this change. Good point, in the future we might expect users complaining that their config files no longer work with FOP's latest release. Still, I think this is preferable for newcomers. Let's just get prepared, after the next release, to answering plenty of times the same question and creating a new FAQ entry about strict user config validation ;-) - the changes in src/documentation/content/xdocs/0.93/fonts.xml are a bit inexact. That's ok, you're probably still learning. But: - the examples you added have different noticeable behaviours: the first one gets the font's metrics through a beforehand generated XML file and won't embed the font in the PDF file, because it doesn't have access to the font file; the second ones computes the metrics on the fly and is able to embed the font in the PDF file. As those differences are not trivial and are explained further on the page I would leave the example as is. I will revert this change. - if both files are specified the metrics-url won't take the precedence, at least not exactly. The metrics defined in this file will override the metrics in the font (if the user modified the file by hand after generating it), but the embed-url file will always be used to load and embed the font. I'm not sure this is the case (but maybe it should be..). All the config fonts are loaded on demand by the LazyFont encapsulation. If you look at org.apache.fop.fonts.LazyFont.load(), the metrics url does take precedence over the embed url (see conditional tests on metricsFileName and fontEmbedPath). Yes, but if you then look at PDFFactory.makeFontFile you will see that the font file is read there for embedding in the resulting PDF file. Your initial wording is essentially right, just a bit misleading: it's true that the data in the metrics file will be given the preference for any font-related computation (glyph placement, kerning, etc.), but finally that's the original font file which is put into the PDF. - AFAIK, kerning is available for PS and PDF outputs. - but as I said, thanks for updating the documentation! - I ask for other devs' opinions, but in strict user-config validation mode I would not also log an error when an exception is subsequently thrown. Reporting the problem just once is enough IMO. I will change this. - In apps.FopFactory: - AFAIU the validatesStrictly() method is not related to user configuration, as the javadoc seems to imply This comment was added to the validatesStrictly method when Simon Pepping commented that these configuration options should be separate and I neglected to remove it. I will now remove this additional method comment. - setUserConfig doesn't throw SAXException or IOException, but FOPException I am at pains to modify public APIs so as to remain backwards compatible and not break anything... There are three public setUserConfig methods, two of which throws SAXException, IOException so I tried to leave these as is - as a FopException is a subclass of SAXException anyways :). The other method needed to be changed though to report an exception (FopException) - instead of just logging it. Ah yes, I forgot that public API issue. Well, that's a bit problematic, then, because backward compatibility is broken anyway (a method which previously didn't throw any exception will now throw one). What's frustrating is that we know the FOPException will only be thrown in strict validation mode, but we must still declare it. So even if the user disabled strict validation, they would still have to catch the exception. H. Personally I would declare a FOPException; if we are to break backward compatibility, we may as well be accurate. But I could understand that other devs don't agree with that. In which case there isn't really a clean solution: embed the FOPException into a RuntimeException, or log a fatal message. What do others think? Vincent
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 [EMAIL PROTECTED] changed: What|Removed |Added Attachment #19498|0 |1 is obsolete|| --- Additional Comments From [EMAIL PROTECTED] 2007-02-07 08:01 --- Created an attachment (id=19539) -- (http://issues.apache.org/bugzilla/attachment.cgi?id=19539action=view) main patch file -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 [EMAIL PROTECTED] changed: What|Removed |Added Attachment #19539|0 |1 is obsolete|| --- Additional Comments From [EMAIL PROTECTED] 2007-02-07 08:11 --- Created an attachment (id=19540) -- (http://issues.apache.org/bugzilla/attachment.cgi?id=19540action=view) main patch file I have updated the main patch file following feedback from Vincent. -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 --- Additional Comments From [EMAIL PROTECTED] 2007-02-07 11:40 --- I had a short look at the patch, and I have the following comments: Do not change the 0.93 documentation but the trunk documentation: src/documentation/content/xdocs/trunk/configuration.xml, etc. TargetResolution is a property of FOUserAgent because it may easily vary with each run. It is not a good idea if you move properties around. They are in FopFactory or in FOUserAgent for a reason. See http://xmlgraphics.apache.org/fop/trunk/embedding.html. Although I did not (and will not) have enough time to look at the patch in detail, I have the feeling that it addresses more than user configuration validation. It does some refactoring. That requires careful inspection. -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 [EMAIL PROTECTED] changed: What|Removed |Added Status|NEW |ASSIGNED -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 --- Additional Comments From [EMAIL PROTECTED] 2007-02-05 03:57 --- Created an attachment (id=19514) -- (http://issues.apache.org/bugzilla/attachment.cgi?id=19514action=view) patch file update for fop config validating xsd file -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 [EMAIL PROTECTED] changed: What|Removed |Added Attachment #19492|0 |1 is obsolete|| --- Additional Comments From [EMAIL PROTECTED] 2007-02-02 03:53 --- (From update of attachment 19492) This patch attachment has been superceeded -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 --- Additional Comments From [EMAIL PROTECTED] 2007-02-02 03:55 --- Created an attachment (id=19497) -- (http://issues.apache.org/bugzilla/attachment.cgi?id=19497action=view) main patch file (In reply to comment #3) The patch is fairly large and seems to address more than only validation of the configuration file. Can you give a summary? The patch seems quite large because I have added a new suite of configuration unit tests for the fix (these are contained in the attached zip file). A summary of the changes is given below. I am against this exception being thrown if I do not use the font arial at all: [ERROR] FOP - Exception org.apache.avalon.framework.configuration.ConfigurationException: Failed to resolve font metric-url 'arial.xml'org.apache.avalon.framework.configuration.ConfigurationException: Failed to resolve font metric-url 'arial.xml' In order for this to be the case, the user configuration would have to be (re-)read/parsed at the runtime of the rendering operation in order that it would be able to know if a particular font is being used in the given FO file. This would be an inefficient way of doing things and would require some structural changes. I am also against combining strict validation of the FO file and the user configuration; they are two different things. I would appreciate a separate option 'validate-configuration', which checks every entry in the configuration file. I did raise this as a question on forum and I remember you responded to it. http://www.nabble.com/Bad-FOP-configurations-tf3064300.html#a8522251 Chris Bowditch suggested that it be combined with strict-validation. I can see your argument for this and this was my original idea but I can also see Chris' argument too. On balance I have decided to go with your suggestion and make it separate a separate configuration option so as to not cause any confusion. I have introduced a new configuration variable called strict-configuration. I haven't called it validate-configuration because validation should always occur, its how the resulting error is handled that is the important thing (see Andreas' comments from the thread). If strict-configuration == false then FOP will simply log the error and continue to parse the user configuration. If strict-configuration == true then FOP will immediately raise an exception and will not continue trying to configure itself. I have attached a new patch file containing these changes (this includes related documentation changes). This new patch file superceeds the previous one. Kind regards, Adrian Cumiskey. -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 [EMAIL PROTECTED] changed: What|Removed |Added Attachment #19497|0 |1 is obsolete|| --- Additional Comments From [EMAIL PROTECTED] 2007-02-02 04:03 --- Created an attachment (id=19498) -- (http://issues.apache.org/bugzilla/attachment.cgi?id=19498action=view) main patch file Includes relevant changes to site (xdocs) documentation. -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 --- Additional Comments From [EMAIL PROTECTED] 2007-02-01 07:32 --- Created an attachment (id=19492) -- (http://issues.apache.org/bugzilla/attachment.cgi?id=19492action=view) main patch file -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
