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

fix(DBComponentField) Swapped to using getValue #37

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nyeholt
Copy link
Contributor

@nyeholt nyeholt commented Feb 22, 2019

Using forTemplate causes some field types to change the underlying value when evaluating the returned data value (ie nl2br, or htmlencoding). This may be a convenient default for some things, but in a general sense causes terrible problems for things like form fields. Those areas that do need it can call the relevant things (like .ATT, .XML) from the template

Using forTemplate causes some field types to change the underlying value when evaluating the returned data value (ie nl2br, or htmlencoding). This may be a convenient default for some things, but in a general sense causes terrible problems for things like form fields. Those areas that do need it can call the relevant things (like .ATT, .XML) from the template
@nyeholt
Copy link
Contributor Author

nyeholt commented Feb 22, 2019

@JoshuaCarter adding you on this because I kinda recall you working on the component stuff recently so you may have some insight as to why it uses forTemplate (cause I don't! :D)

@JoshuaCarter
Copy link
Contributor

My exposure was limit to the ComponentService area of things.

That said, a quick squiz at the default DBField types suggests that DBString is the only one doing something in forTemplate that would be impacted by this change (return nl2br(parent::forTemplate());).

Have you tested this with DBString? If not, it might be worth testing, and if needed forTemplate can be used just for DBString?

@nglasl
Copy link
Contributor

nglasl commented Oct 1, 2019

@nyeholt this one needs your input before it can be merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants