Closed (fixed)
Project:
Views Bonus Pack
Version:
6.x-1.x-dev
Component:
Views Export
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
5 Feb 2009 at 22:48 UTC
Updated:
6 Aug 2010 at 12:50 UTC
Jump to comment: Most recent file
Comments
Comment #1
neclimdulI'm tentatively going to call this by design. I don't want to fudge with the values and cause unexpected changes to the values of the elements(even if it means causing invalid elements). I believe there is some responsibility on the user to set the titles correctly, even if that commonly means overriding the values.
Comment #2
markj commentedYour call, but since the XML that is exported is not well formed (it won't even display in a web browser), maybe it shouldn't be called an 'XML export'. Maybe 'marked up' or 'tagged'?
Comment #3
dman commentedTotally. We need people to stop abusing XML...
Get it right early on, and it will be future-proof. Allow quirks through and it will fail.
How about :
... actually, there's lots of room for improvement, like ....
<published name="Published" value="1" />... and is that title really supposed to have the href embedded?
Comment #4
neclimdulI think you miss understand my reason for saying this is by design. I understand that that XML is not well formed but it is only not well formed because the label you have set for the field doesn't make for well formed XML. You can change the label to something well formed simply by overriding the label for you XML output display.
Along those lines, the link could be removed by overriding the option on the field.
As far as doing more complicated things like attributes, I'd be open to investigating this in a feature request. However, I was mostly focusing on providing a basis for simple XML feeds to be built form and this might be something more fitting for Views Datasource.
Comment #5
markj commentedSorry, I didn't know it was possible to override the labels for the XML output. How do I do that, using a custom field label in my XML feed's display configuration?
Thanks for pointing out Views Datasource, it looks useful.
Comment #6
markj commentedJust answering my own question -- yes, I was able to make all element names well formed by assigning XML-feed specific field labels. Sorry I didn't think of this before opening the issue.
Comment #7
WorldFallz commentedI get why you consider this to be by design, but having just spent a lot of time overriding the default labels of 100+ fields (often forgetting to click the 'override' button), could i persuade you to consider an option in the xml style settings like the "Transform spaces to dashes in URL" option in views arguments?
Let me know if you would consider this, and I'll try to roll patch for it.
Comment #8
neclimdulI'm always open for good ideas. Lets see how it'd work.
Comment #9
WorldFallz commentedok, let me roll a rough patch and we'll see where it goes. thanks.
Comment #10
WorldFallz commentedHere's what I have so far, now I just have to figure out how to actually implement the string replace ;-)
Comment #11
WorldFallz commentedOk, here's a full functioning implementation. There's a couple of ways I could have implemented the string replace, I did it the simplest way until I see how you feel about the patch.
Comment #12
neclimdulLooks pretty reasonable.
A couple of nitpicks. I'm pretty sure this options isn't translatable. Also, I'm always hesitant to roll out changes that affect the default functionality(ie defaulting no features that mangle output to true). I guess its probably ok in this case since it would only affect people outputting broken XML. Lets make sure we think it over though.
It also feels like maybe if we're going to do this, we should go all out and have a couple options. ie. '-', '_', and maybe camel. I'll leave how far you want to take the implementation in the patch up to you though.
Comment #13
WorldFallz commentedGood points. I also thought about adding options for '_' and camel but in the end took the easy way out, lol. I also thought about changing the actual application of the option to a preprocess function instead of in the template file, but I couldn't get it to work for some reason. I'll work on it some more and see how far I can get. And thanks for considering the patch!
Comment #14
neclimdulYeah, you'll need to move that label "building" code out of the template and into the preprocess function in views_bonus_export.theme.inc creating a new "label" key in $vars
Probably have to do something like
Then use this in the template:
All code totally out of my head and not tested, if that "magic process logic" didn't give it away.
Comment #15
WorldFallz commentedquick question-- which is more common in xml, camelCase or PascalCase?
Comment #16
WorldFallz commentedOK, a new patch with options for '-', '_', and PascalCase.
Comment #17
WorldFallz commentedand thanks for the hint about the preprocess function... that'll be in the next patch.
Comment #18
neclimdulre common usage: don't really know. I've come across both depending on who i'm developing for but I'd guess camelCase would be pretty important based on that fact RSS uses it(ie pubDate)
some nits
'translatable' => TRUE, should still be FALSE
you need to provide 'transform_type' in your options_definition function
Nice use of the dependent field :) Exactly how I'd want it.
Comment #19
WorldFallz commentedok, this is for real-- should be ready to go. Let me know if i missed anything.
Comment #20
WorldFallz commentedmeh... I missed your post. Let me make those changes.
Comment #21
WorldFallz commentedok, for real for real this time, lol. Includes all comments so far.
Comment #22
WorldFallz commentedwhat the heck, I hate incomplete things, lets add camelCase:
Comment #24
WorldFallz commentedyet one last one-- somehow the original hunk for the tpl,php file keeps showing up, this one removes it (and fixes a bug with blank $labels).
Comment #25
WorldFallz commentedhmm... same problem. Trying one more time. If this doesn't work, just delete that hunk, lol.
Comment #26
neclimdulAwesome work, I'll try and give it an actual honest to god apply and look over review tonight or tomorrow. Today got spent with a spawn from this issue #70719: Add Unicode::ucwords() and Unicode::lcfirst() implementation and make use of them in core. Though I don't really care to much about unicode support ATM so please don't worry about implementing that patch here :)
Comment #27
WorldFallz commentedsounds good.
Comment #28
neclimdulLooking to commit a slightly modified version. Will do it in the morning if you don't see anything glaringly wrong.
Changes:
Comment #29
WorldFallz commentedfyi there's some csv hunks in your patch but it looks great-- and I think both changes make sense.
Comment #30
neclimdulCommitted without csv hunks :)
Comment #31
WorldFallz commentedawesome-- thanks!
Comment #32
WorldFallz commentedlooks like the patch got mangled somehow. I just checked out the dev version, and the changes to views_bonus_plugin_style_export_xml.inc are repeated a few times in the file. I deleted the repeats, and otherwise it works great-- thanks again.
Comment #33
neclimdulweird. Removed and should be fine now. Thanks for catching that.