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

Added a simple token migration for dashes #390

Closed
wants to merge 1 commit into from

Conversation

Toflar
Copy link
Member

@Toflar Toflar commented Jan 15, 2025

This automatically migrates simple tokens using - to using _. The alternative would be to allow both, - as well as _ but I'm still not sure this is a good idea from a UX perspective...

Background: Contao's SimpleTokenParser (or Symfony Expression Language) cannot handle tokens with -. Thus, you cannot use them in {if conditions. Hence, there are 2 solutions:

  1. Normalize all tokens from - to _ and migrate existing data (current solution with this PR)
  2. Automatically generate normalized tokens on top of e.g. ##e-mail## so that you can write {if e_mail ...}##e-mail##{endif}

So 1. would have consistent tokens everywhere but force users to re-think somehow. With the migration in this PR it would be complete, though. 2. would have inconsistent tokens but maybe this is better?

@fritzmg
Copy link
Collaborator

fritzmg commented Jan 15, 2025

I think UX wise there cannot be a perfect solution - unless Contao disallows field names with - in the first place (or the parser allows tokens with - in conditions).

Even with this migration in place, users still can create fields with - in its name and thus likely insert ##foo-bar## in the notification and it won't work. Only after they happen to execute the database migration afterwards it would.

And conversely, if instead the NC allows to output tokens like ##foo-bar## the output will work, but the user will wonder why {if foo-bar} does not output anything.

@Toflar
Copy link
Member Author

Toflar commented Jan 15, 2025

The alternative would be this: #391

@Toflar
Copy link
Member Author

Toflar commented Jan 15, 2025

I think we should go for #391. It will make sure existing notifications will work just fine if there were no conditionals. The conditionals need to be updated anyway but they also throw an error so people would notice during a migration. But it's a fact that echoing statements (so just ##foobar##) are used way more often than conditional statements.
Also, @fritzmg's statement about the migration solving it for updates but then users will still continue to write ##foo-bar## is also an important one with which I agree.

@Toflar
Copy link
Member Author

Toflar commented Jan 16, 2025

I decided #391 is the better approach :)

@Toflar Toflar closed this Jan 16, 2025
@Toflar Toflar deleted the fix/simple-token-migration branch January 16, 2025 09:16
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.

2 participants