Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow multiple datasource parameters #728

Closed
brendo opened this issue Jul 31, 2011 · 36 comments
Closed

Allow multiple datasource parameters #728

brendo opened this issue Jul 31, 2011 · 36 comments
Assignees
Milestone

Comments

@brendo
Copy link
Member

brendo commented Jul 31, 2011

At the moment datasources can only output a single parameter and while you can add additional parameters by customising the datasource (or worse creating another), it's not efficient and really should be out of the box

@ghost ghost assigned brendo Jul 31, 2011
@nickdunn
Copy link
Contributor

For the record, I added this many moons ago to a 2.0.3 (or so) version. Happy to pass on the code if I can find it. Worked as expected EXCEPT for dependency/ordering resolution which, at the time, relied upon output parameter names to be in the form $ds-{name}, so adding new names such as $ds-{name}-{field} meant that dependency wouldn't be evaluated properly and ordering was wrong. But not a massively difficult one to fix.

This would be the awesomes.

@brendo
Copy link
Member Author

brendo commented Jul 31, 2011

Definitely would be helpful!

@ahwayakchih
Copy link
Contributor

I also wrote extension lately that allows to output additional parameters from the XML that was generated. Very useful when using dynamic XML data-sources, but also in other situations.

@brendo
Copy link
Member Author

brendo commented Aug 1, 2011

Can you open a separate issue for that @ahwayakchih? I'd like to look at including it, there has been a number of times where that functionality would of been incredibly useful.

@ahwayakchih
Copy link
Contributor

Sure i can and will, but it's important to note here, that i also hit the same problem that nickdunn hit. To keep order of data-sources i have to use {$ds-datasource} somewhere, where it's not important, or keep default parameter empty and use {$ds-datasource:$ds-datasource-xpath-0} (default empty, so it falls back to my parameter, but Symphony sees default parameter used and reorders data-sources the way i wanted to :).

So: it would be great if core did not depend on a parameter being single value (dsParamPARAMOUTPUT could be associative array) and if it could check list of output parameters to get list of parameter names that will be created, instead of checking for hardcoded, single $ds-datasourcename when reordering data-sources.

@brendo
Copy link
Member Author

brendo commented Aug 1, 2011

Yep, that's one potential solution.

@brendo
Copy link
Member Author

brendo commented Aug 9, 2011

Just an update. I've implemented multiple parameters whilst maintaining backwards compatibility.

Currently parameters are set as $ds-datasource-handle, and the dependency is set as $ds-datasource-handle as well. Multiple parameters are output a little differently, $ds-datasource-handle.field-handle, with the dependency still set as $ds-datasource-handle.

Now the conundrum is that while keeping $ds-datasource-handle alive to maintain backwards compatibility, it's still not perfect as if/when a developer selects a second parameter, they will still have to update their XSLT/PHP/other datasources to look for $ds-datasource-handle.field-handle instead of $ds-datasource-handle.

What I'm thinking of doing is mirroring $ds-datasource-handle with the $ds-datasource-handle.field-handle, so developers can safely use that syntax in their 2.3 sites without worrying about the syntax.

I'll push this code as soon as 2.2.3 is out (integration is dedicated to that at the moment)

@ahwayakchih
Copy link
Contributor

Why not make data-sources list parameters they output? It would stay backwards compatible and at the same time it would not need to depend on field-handle (it still could, but it would not be required). AFAIK there's only one place in Symphony that would have to be changed, and it could simply check if dsParamOUTPUT is array or not. Old data-sources would not have to be updated at all.

@brendo
Copy link
Member Author

brendo commented Aug 10, 2011

Datasources already do list parameters :)

@ahwayakchih
Copy link
Contributor

dsParamOUTPUT is not an array. It could be an associative array, where keys are parameter names and values are field names or whatever else extensions put there (BTW it would be great if pre-save delegate was called on data-source before its code and things like EXTRAS are parsed, so extensions can add/change things easy way, instead of re-writing PHP :).

Code that reorders data-sources could then check all parameter names. There would be no need for hardcoded name-checks.

@brendo
Copy link
Member Author

brendo commented Aug 10, 2011

Hm, we might be misunderstanding each other, dsParamOUTPUT is not array currently, but with the work I've been doing it now is treated as an array regardless. So if it's a string, it'll just be wrapped as an array.

As for the delegate, does DatasourcePreCreate not cut the mustard?

Code that reorders data-sources could then check all parameter names. There would be no need for hardcoded name-checks.

Not sure what you mean here, could you explain in a different way?

@ahwayakchih
Copy link
Contributor

So if it's a string, it'll just be wrapped as an array.

Nice :).

DatasourcePreCreate is called after Symphony generates PHP code from the template file. So there is no way to add variables, modify existing ones, add code into "EXTRA" part (defined in template, but i do not think it is used by section data-sources) of grab function, etc...

I had to tokenize PHP to inject own code there :).

Not sure what you mean here, could you explain in a different way?

Symphony code assumes that there is always $ds-datasourcehandle parameter (or none at all). If data-sources had defined list of parameter names they output, it could just check that list instead of hardcoded, single parameter name per data-source.

So, a separate list similar to the list of data-source dependencies, or dsParamOUTPUT turned into associative array, where each key is parameter name.

brendo added a commit to brendo/symphony-2 that referenced this issue Aug 13, 2011
@nickdunn
Copy link
Contributor

What I'm thinking of doing is mirroring $ds-datasource-handle with the $ds-datasource-handle.field-handle, so developers can safely use that syntax in their 2.3 sites without worrying about the syntax.

While this makes sense now, it will be confusing in the XML output list of params. You would get the value repeated in the XML under two different names, I presume?

@brendo
Copy link
Member Author

brendo commented Aug 16, 2011

Yeah unfortunately it will. We could prevent this from happening though if necessary :)

You could pull in my fork above to test this out now, I'm currently using this in a personal project so it's a getting a bit thorough workout :D

@brendo
Copy link
Member Author

brendo commented Aug 17, 2011

I've pulled this into integration now. I'd say it's pretty reliable at the moment, (as in, it's been working in a side project for two weeks reliably), but it would be still good to get feedback.

I haven't yet made any amends for the 'mirroring' just yet.

@brendo
Copy link
Member Author

brendo commented Aug 29, 2011

That would be where @eKoeS and @nilshoerrmann step in and make things sexy :)

@nilshoerrmann
Copy link
Contributor

We'll take a look at it :)

@simoneeconomo
Copy link
Contributor

That would be where @eKoeS and @nilshoerrmann step in and make things sexy :)

[Assigned to myself so that I remember about this]

@ghost ghost assigned simoneeconomo Oct 22, 2011
@simoneeconomo
Copy link
Contributor

I think it needs to tell you what the parameters will be called as you select them.

I'm not sure the backend is the right place for that. It's pretty difficult to explain in a few lines which is the value of ?. The only thing that comes to my mind is:

The parameters $ds-ds_name.? will be created with this field’s value for XSLT or other data sources to use, with ? being the field handles.

Any ideas for a better phrase?

@simoneeconomo
Copy link
Contributor

And it needs to rename the parameters as you re-type the Datasource name.

The problem here is that, in order to generate a valid handle, we should either have a JS-equivalent of transliterations or make an AJAX request for each keypress event (!) -- unless we use change.

@brendo
Copy link
Member Author

brendo commented Nov 17, 2011

The parameters $ds-ds_name.? will be created with this field’s value for XSLT or other data sources to use, with ? being the field handles.

I should point out that it's more like $ds-?.field-handle because we already know the field handles for the given section, what we don't know is the datasource name (on creation anyway).

The problem here is that, in order to generate a valid handle, we should either have a JS-equivalent of transliterations or make an AJAX request for each keypress event (!) -- unless we use change.

That sounds like it could be useful in other areas as well! I think using change/focusout will work nicely

@nilshoerrmann
Copy link
Contributor

We shouldn't use an AJAX request on keypress. That might result in a really huge ammount of requests.

@brendo
Copy link
Member Author

brendo commented Nov 17, 2011

I think change is the go.

@simoneeconomo
Copy link
Contributor

We shouldn't use an AJAX request on keypress. That might result in a really huge ammount of requests.

Yep, that was the point :)

@simoneeconomo
Copy link
Contributor

So, if I understood it correctly: AJAX request on change event fired?

@brendo
Copy link
Member Author

brendo commented Nov 20, 2011

Yeah, although it doesn't need to be AJAX, it's just taking the value of that field and replacing the ? in the supporting copy about parameters :)

@simoneeconomo
Copy link
Contributor

Yeah, although it doesn't need to be AJAX, it's just taking the value of that field and replacing the ? in the supporting copy about parameters :)

Okay, I got lost :) Are we talking about the datasource handle or the field handle? I thought Rowan was referring to the first, which is why I mentioned AJAX et al.

@brendo
Copy link
Member Author

brendo commented Nov 22, 2011

It's the datasource handle. Rowan had a typo. From above:

I should point out that it's more like $ds-?.field-handle because we already know the field handles for the given section, what we don't know is the datasource name (on creation anyway).

Parameters will be named $ds-read-articles.system-id or $ds-read-articles.tags. We know $ds- and we know all the available field handles. It's the read-articles handle that is the unknown (until a user names the datasource)

@simoneeconomo
Copy link
Contributor

It's the read-articles handle that is the unknown (until a user names the datasource)

Right, but in this case we either make an AJAX request to generate the handle or we re-implement the Lang::createHandle() function in JavaScript. Are there any other ways I didn't think of?

Concerning parameters, my suggestion is to have them displayed as a list. What do you think?

@brendo
Copy link
Member Author

brendo commented Nov 22, 2011

AJAX request to generate the handle

Is probably the best way, then we only have to maintain one handle function.

Concerning parameters, my suggestion is to have them displayed as a list. What do you think?

I like the S3 behaviour here. The actual option labels update from $ds-?.field-handle to be $ds-read-articles.field-handle. It displays the information in a concise way rather than repeating what the user has selected.

@simoneeconomo
Copy link
Contributor

The actual option labels update from $ds-?.field-handle to be $ds-read-articles.field-handle. It displays the information in a concise way rather than repeating what the user has selected.

What if the user selects more than one field as parameter?

@brendo
Copy link
Member Author

brendo commented Nov 22, 2011

It's a <select> with multiple selected. Screenshots:

New DS
Edit DS

@simoneeconomo
Copy link
Contributor

Ah, sorry! I was talking about the help text. This explains why I couldn't get the whole point of this discussion. :) Now, bed time. Will let you know how it goes.

@nilshoerrmann
Copy link
Contributor

It still looks like this on my install:

Parameter Output

Am I missing something?

@brendo
Copy link
Member Author

brendo commented Nov 27, 2011

The screenshots were from S3 :)

@nilshoerrmann
Copy link
Contributor

Aha, that makes a lot more sense.

In this case I'd like to suggest to use that layout for S2 as well. We don't need that explaining copy and we can just label the select box "Parameter Output" (which would correspond nicely with the right column).

simoneeconomo added a commit to simoneeconomo/symphony-2 that referenced this issue Nov 27, 2011
simoneeconomo added a commit that referenced this issue Dec 1, 2011
brendo added a commit that referenced this issue Dec 1, 2011
@brendo brendo closed this as completed Dec 2, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants