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 incorrect twig field checks for contacts and companies #13254

Merged
merged 1 commit into from Jan 20, 2024

Conversation

mollux
Copy link
Contributor

@mollux mollux commented Jan 20, 2024

Q A
Bug fix? (use the a.b branch) [x]
New feature/enhancement? (use the a.x branch) [ ]
Deprecations? [ ]
BC breaks? (use the c.x branch) [ ]
Automated tests included? [ ]
Related user documentation PR URL mautic/mautic-documentation#...
Related developer documentation PR URL mautic/developer-documentation#...
Issue(s) addressed Fixes #13197

Description:

during the phptemplate to thig migration, some checks were not correcty converted, resulting in 500 errors when a field doesn't exist.
As Mautic allows to disable specific fields via the UI, this is a common usecase.

in PHP this works

if (!empty($fields['core']['city']['value'])):
    $location[] = $fields['core']['city']['value'];
endif;

but the equivalent in twig doesn't work, and throws a Twig\Error\RuntimeError

{% if fields.core.city.value is not empty %}
   {% set location = location|merge([fields.core.city.value]) %}
{% endif %}

This PR addresses this, by checking if the parent is defined.

questions for someone more familiar with twig

what is the proper way to check this?
is it

  • {% if fields.core.city is defined %}
  • {% if fields.core.city is defined and fields.core.city.value is defined %}
  • {% if fields.core.city is defined and fields.core.city.value is defined and fields.core.city.value is not empty %}
  • another option

This makes me wonder is we should provide a generic does exist is is not empty helper, to be able to check properly the whole structure?

Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. disable all fields except email, company name
  3. create contacts, create companies, add contacts to companies, list company details, ...
  4. verify no 500 Twig\Error\RuntimeError errorss are thrown

@mollux mollux added bug Issues or PR's relating to bugs ready-to-test PR's that are ready to test twig labels Jan 20, 2024
@mollux mollux added this to the 5.0.3 milestone Jan 20, 2024
@mollux mollux linked an issue Jan 20, 2024 that may be closed by this pull request
1 task
@escopecz escopecz added contacts Anything related to contacts companies Anything related to companies labels Jan 20, 2024
@escopecz escopecz self-requested a review January 20, 2024 14:12
Copy link
Sponsor Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

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

Works as described. No error. I can create new contact and companies, edit them link them. We should probably disable unpublishing of company name as it's an identificator field that must be present. Next time...

Thank you! 👍

@escopecz escopecz merged commit a0e2f2b into mautic:5.0 Jan 20, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues or PR's relating to bugs companies Anything related to companies contacts Anything related to contacts ready-to-test PR's that are ready to test twig
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

HTTP 500 when loading contacts details page after upgrade to 5.0.1
2 participants