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

Modded core's select field to accommodate for multiple contenttypes. #6270

Conversation

graham73may
Copy link
Contributor

This modification allows core's select fields to handle multiple content types. This is already possible within the codebase because value is applied directly to the setcontent call in the _select.twig template. Therefore the first part of value could have any valid setcontent input. It just didn't work because the existing select field combined all the ID's.

This small change works by checking for the existence of contenttype and if it exists, prepending it to the output text and also prepending it to the output values. This will work for singular or multiple content type combinations and has no effect on existing select fields as before declaring contenttype would have returned an exception.

An example of the output with contenttype declared:
image

An example of the value/text pairs generated:
image

The contenttypes.yml example:

featured_items:
    label: "Event/Event series"
    autocomplete: true
    type: select
    values: (event,eventseries)/contenttype,title

(The two contenttypes being shown are event and eventseries)

Of course the output style could be changed if anyone would prefer a different format!

Target branch

  • release/3.2 — "stable" branch

@bobdenotter
Copy link
Member

Can you make this PR against release/3.3? No new features in the 3.2.x branch anymore. :-)

@GwendolenLynch GwendolenLynch changed the base branch from release/3.2 to release/3.3 January 18, 2017 14:38
@GwendolenLynch
Copy link
Contributor

Can you make this PR against release/3.3

Like magic 😉

@bobdenotter
Copy link
Member

bobdenotter commented Jan 18, 2017

Ok, gawain magic made it against release/3.3.. there's a merge conflict now though, so if you could look into that, @graham73may ..

@GwendolenLynch
Copy link
Contributor

Just needs a rebase … call out if you need help with that.

@graham73may
Copy link
Contributor Author

graham73may commented Jan 18, 2017

@GawainLynch Rebased on to 3.3 and pushed. Some other file changes got flagged so undid those and pushed again.

What's caused all the conflicting files :/ ?

@GwendolenLynch GwendolenLynch force-pushed the feature__select-multiple-contenttypes branch from 32069fb to 4ab588d Compare January 18, 2017 15:25
@SvanteRichter
Copy link
Contributor

SvanteRichter commented Jan 18, 2017

For the future (4.0 I guess) I think we should standardize this to one format. Having the field return completely differently formatted values for values: (event,eventseries)/contenttype,title vs. values: eventseries/id,title makes no sense to me.

@SvanteRichter
Copy link
Contributor

Also, from what I can see from the docs (and from memory) the fieldnames (what comes after the slash) was never meant to be more than presentational, ( https://docs.bolt.cm/3.2/fields/select#populating-the-values-from-a-contenttype ), so having them determine functionality is a bit odd since values: (event,eventseries)/contenttype,title and values: (event,eventseries)/id,title would produce very different values, but the only change is in the presentational part of the config.

@mrenigma
Copy link

@SahAssar you're right, the right hand side should only be for presentation reasons. How about instead of determining the value format from the presence of contenttype we instead base it off whether there are more than one content types defined?

@SvanteRichter
Copy link
Contributor

SvanteRichter commented Jan 18, 2017

@mrenigma Well, the base problem is that we have crammed so much functionality into the select fieldtype that it's getting hard to manage. Adding yet another format to both the config and values seems to me like it's handing all of us another (though very attractive, useful and wanted, but still) footgun.

I'd prefer if this was spun off as a separate fieldtype (still in core) called contentselect or whatever and we deprecate having contenttype values in the normal select field (for removal in 4.0). Then we could also say that the contentselect fieldtype will always have an array of {contenttype}/{id} values, which would make it easier for template authors, config validation and whatnot. Does that sound doable and reasonable to everyone?

EDIT: It'd also ease getting some order into the select fieldtype template, which is sortof a mess.

@mrenigma
Copy link

mrenigma commented Jan 18, 2017

@SahAssar Being devils advocate here - you could just make the select field always save as {contenttype}/{id} instead of just {id}. We currently have a select field in core which leverages the setcontent functionality - so by that logic select will always be able to do a lot!

If it is split off, I would propose simplifying the select field to only static arrays of either values or key/value pairs and then move all the contenttype search functionality over to the new contentselect instead.

@mrenigma
Copy link

Also worth saying that if in 4.0 everyone has to rename some of their config fields from select to contentselect there will be more work involved for the fields under repeaters as the field type is stored in the DB table for those as well (not entirely sure why this is...). And from experience if the field type in the config doesn't match the one in the DB you get errors.

@SvanteRichter
Copy link
Contributor

@mrenigma Well, we could if we were willing to break backwards compatibility in a minor version, as it stands now the select field must be able to accept to values/configs it has during 3.X, until we release 4.X.

The split off basically makes it so that we can force the select fieldtype to only be simple arrays and hashes, while keeping any more advanced, contenttype-specific functionality in a certain fieldtype, so that's my thought.


My biggest issues with the select field is that

  • the config changes format based on if it is contenttype/value
  • the value changes format based on if it is contenttype/value and multiple/single
  • the UI changes quite a bit based on if it is multiple/single and sortable/non-sortable

So basically everything can change, but still be called the same field. If that same philosophy was used for other fields text, textarea, hidden, wysiwyg and markdown would all be one fieldtype called text with options attached.


But don't take my word for it until some other folks have chimed in, this is just my opinion 😄

@GwendolenLynch GwendolenLynch changed the base branch from release/3.3 to release/3.4 January 24, 2017 18:50
@GwendolenLynch
Copy link
Contributor

OK, called good to go at tonight's meeting.

@GwendolenLynch GwendolenLynch merged commit edca9fd into bolt:release/3.4 Jan 24, 2017
@GwendolenLynch GwendolenLynch added this to the Bolt 3.4 - Feature release milestone Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants