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.

Sign up for GitHub

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

Jump to bottom

FEATURE: Add enableReferrer and enableTrustedProperties to the Form Definition #64

Merged
merged 2 commits into from
May 2, 2024
Merged

FEATURE: Add enableReferrer and enableTrustedProperties to the Form Definition #64

merged 2 commits into from
May 2, 2024

Conversation

mficzel
Copy link
Member

@mficzel mficzel commented Oct 29, 2021 •
edited

FEATURE: Add enableReferrer and enableTrustedProperties to the prototype Neos.Fusion.Form:Definition.Form
Both settings are disabled by default which does not change the current behavior.

  • enableReferrer: (bool, defaults to true) Enable the generation of hidden __referrer fields. Can be disabled when the method is get or no flow validation is used
  • enableTrustedProperties: (bool, defaults to true) Enable the generation of hidden __trustedProperties fields. Can be disabled when flow property mapping is not in use

In Addition the RuntimeForm now sets enableReferrer to false because the runtime form only uses trusted properties but will never redirect back to the referrer.

Resolves: #73

@mficzel mficzel requested a review from grebaldi October 29, 2021 16:27
@mficzel mficzel force-pushed the feature/makeReferrerFieldsOptional branch from b234c4f to 3a42ce2 Compare October 29, 2021 17:00
@mficzel mficzel removed the request for review from grebaldi October 29, 2021 17:00
@mficzel
Copy link
Member Author

mficzel commented Oct 29, 2021

While i am pretty sure that the rendering of the referrer fields should be controllable i am not totally sure the chosen default to only render for post makes sense.

Flow renders the referrers fields always so it might also make sense to set addReferrers to true by default.

@mficzel mficzel force-pushed the feature/makeReferrerFieldsOptional branch 4 times, most recently from 33b58c0 to 9f36b3d Compare October 6, 2023 16:20
@mficzel mficzel changed the title FEATURE: Make referrer fields optional FEATURE: Add disableReferrer and disableTrustedProperties to the Form Definition Oct 6, 2023
@mficzel
FEATURE: Add to disableReferrer and disableTrustedProperties to F…
9a9cb73 
…ormDefinition

Both settings are disabled by default which does not change the current behavior.

- `disableReferrer` avoids rendering referrers for endpoints that do not use flow validation, use method get or
- `disableTrustedProperties` avoids rendering rendering trusted properties tokens in cases where property mapping is not used

In Addition the RuntimeForm now sets `disableReferrer` because the runtime form only uses trusted properties but will never redirect back to the referrer.
@mficzel mficzel force-pushed the feature/makeReferrerFieldsOptional branch from 9f36b3d to 9a9cb73 Compare October 6, 2023 16:40
@mficzel mficzel requested a review from jonnitto October 6, 2023 16:42
mikec655
mikec655 approved these changes May 2, 2024
View reviewed changes
Copy link

@mikec655 mikec655 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mficzel I don't see how you would improve this. It is a simple and straight forward solution. Making it more elegant won't make it better in my opinion.

@kitsunet
Copy link
Member

kitsunet commented May 2, 2024

disableSomething=true I find always nasty, I would rather have something=false (and the default is something=true) for this case. BUT I don't want to block this change, IMHO we could merge it as is if you don't want to change that.

@mficzel
Copy link
Member Author

mficzel commented May 2, 2024 •
edited

@kitsunet we could change this to renderReferrer and renderTrustedProperties which would be enabled by default would you consider that beeing better?

@bwaidelich
Copy link
Member

we could change this to renderReferrer and renderTrustedProperties

I like the direction that prevents those double negatives. Maybe we can go one step further and call them renderReferrerField und renderTrustedPropertiesField?

@mficzel mficzel changed the title FEATURE: Add disableReferrer and disableTrustedProperties to the Form Definition FEATURE: Add enableReferrer and enableTrustedProperties to the Form Definition May 2, 2024
@mficzel
Copy link
Member Author

mficzel commented May 2, 2024 •
edited

@bwaidelich changed disable to enable which indeed makes more sense and avoids disable=false double negatives . If noone objects i will merge and create a feature release soon. This pr is way to old to my taste.

bwaidelich
bwaidelich approved these changes May 2, 2024
View reviewed changes
Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 by 👁️ thanks!

@mficzel mficzel merged commit 75e0800 into neos:main May 2, 2024
7 checks passed
@mficzel
Copy link
Member Author

mficzel commented May 2, 2024

merged and released as 2.3.0

@mikec655 thanks for triggering this. You will have to adjust your code to the disable > enable switch but this is finally merged.

@mikec655
Copy link

mikec655 commented May 2, 2024

Thanks a lot for the effort everyone

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

@bwaidelich bwaidelich bwaidelich approved these changes

@mikec655 mikec655 mikec655 approved these changes

@kitsunet kitsunet Awaiting requested review from kitsunet

@jonnitto jonnitto Awaiting requested review from jonnitto

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make adding referrer fields optional
4 participants
@mficzel @kitsunet @bwaidelich @mikec655