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

Add validation to ensure that we are not trying to access attributes … #9795

Merged
merged 2 commits into from May 21, 2021

Conversation

mohit-rocks
Copy link
Contributor

@mohit-rocks mohit-rocks commented Mar 17, 2021

Q A
Branch? "features" for all features, enhancements and bug fixes (until 3.3.0 is released)
Bug fix? yes
New feature? no
Deprecations? no
BC breaks? no
Automated tests included? no
Related user documentation PR URL mautic/mautic-documentation#...
Related developer documentation PR URL mautic/developer-documentation#...
Issue(s) addressed Fixes #9762

Description:

Scenario 1:

Scenario 2:
Comment: #9795 (comment)

Scenario 3:
Comment: #9795 (comment)

Steps to test this PR:

  1. Load up this PR

@cla-bot cla-bot bot added the cla-signed The PR contributors have signed the contributors agreement label Mar 17, 2021
@mohit-rocks mohit-rocks marked this pull request as ready for review March 18, 2021 04:56
@mohit-rocks
Copy link
Contributor Author

The other two issues mentioned in #9762 are not reproducible anymore.
I think it needs some testing from community folks.

@codecov
Copy link

codecov bot commented Mar 18, 2021

Codecov Report

Merging #9795 (a6a9cf2) into features (b84c8f4) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##             features    #9795   +/-   ##
===========================================
  Coverage       41.25%   41.25%           
- Complexity      34563    34566    +3     
===========================================
  Files            2060     2060           
  Lines          111535   111535           
===========================================
  Hits            46014    46014           
  Misses          65521    65521           
Impacted Files Coverage Δ Complexity Δ
...le/Form/DataTransformer/FieldFilterTransformer.php 50.00% <0.00%> (ø) 18.00 <0.00> (+1.00)
...dles/ReportBundle/Form/Type/FilterSelectorType.php 0.00% <0.00%> (ø) 8.00 <0.00> (+2.00)

@RCheesley RCheesley added bug Issues or PR's relating to bugs dynamic-content email Anything related to email essential This must be done to close the milestone regression A bug that broke something in the last release segments Anything related to segments T1 Low difficulty to fix (issue) or test (PR) labels Mar 19, 2021
@mautibot
Copy link

This pull request has been mentioned on Mautic Community Forums. There might be relevant details there:

https://forum.mautic.org/t/notices-trying-to-access-array-offset-on-value-of-type-null/18008/9

Copy link
Sponsor Member

@RCheesley RCheesley left a comment

Choose a reason for hiding this comment

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

@mohit-rocks This seems to resolve the issue for email - creating a new email using the Goldstar theme I no longer see the errors.

DWC does seem to have been fixed as that is no longer throwing an error for me.

Report saving is now throwing a different error for me:

[2021-03-19 19:18:13] mautic.CRITICAL: Uncaught PHP Exception Symfony\Component\Debug\Exception\ContextErrorException: "PHP Warning - Illegal string offset 'column'" at /var/www/html/app/bundles/ReportBundle/Form/Type/FilterSelectorType.php line 88 {"exception":"[object] (Symfony\\Component\\Debug\\Exception\\ContextErrorException(code: 0): PHP Warning - Illegal string offset 'column' at /var/www/html/app/bundles/ReportBundle/Form/Type/FilterSelectorType.php:88)
[stacktrace]
#0 /var/www/html/app/bundles/ReportBundle/Form/Type/FilterSelectorType.php(88): Mautic\\CoreBundle\\ErrorHandler\\ErrorHandler->handleError(2, 'PHP Warning - I...', '/var/www/html/a...', 88, Array)
#1 /var/www/html/vendor/symfony/event-dispatcher/EventDispatcher.php(214): Mautic\\ReportBundle\\Form\\Type\\FilterSelectorType->Mautic\\ReportBundle\\Form\\Type\\{closure}(Object(Symfony\\Component\\Form\\FormEvent), 'form.pre_bind', Object(Symfony\\Component\\EventDispatcher\\EventDispatcher))
#2 /var/www/html/vendor/symfony/event-dispatcher/EventDispatcher.php(44): Symfony\\Component\\EventDispatcher\\EventDispatcher->doDispatch(Array, 'form.pre_bind', Object(Symfony\\Component\\Form\\FormEvent))
#3 /var/www/html/vendor/symfony/event-dispatcher/ImmutableEventDispatcher.php(33): Symfony\\Component\\EventDispatcher\\EventDispatcher->dispatch('form.pre_bind', Object(Symfony\\Component\\Form\\FormEvent))
#4 /var/www/html/vendor/symfony/form/Form.php(552): Symfony\\Component\\EventDispatcher\\ImmutableEventDispatcher->dispatch('form.pre_bind', Object(Symfony\\Component\\Form\\FormEvent))
#5 /var/www/html/vendor/symfony/form/Form.php(573): Symfony\\Component\\Form\\Form->submit(Array, true)
#6 /var/www/html/vendor/symfony/form/Form.php(573): Symfony\\Component\\Form\\Form->submit(Array, true)
#7 /var/www/html/vendor/symfony/form/Extension/HttpFoundation/HttpFoundationRequestHandler.php(109): Symfony\\Component\\Form\\Form->submit(Array, true)
#8 /var/www/html/vendor/symfony/form/Form.php(487): Symfony\\Component\\Form\\Extension\\HttpFoundation\\HttpFoundationRequestHandler->handleRequest(Object(Symfony\\Component\\Form\\Form), Object(Symfony\\Component\\HttpFoundation\\Request))
#9 /var/www/html/app/bundles/CoreBundle/Controller/AbstractFormController.php(167): Symfony\\Component\\Form\\Form->handleRequest(Object(Symfony\\Component\\HttpFoundation\\Request))
#10 /var/www/html/app/bundles/ReportBundle/Controller/ReportController.php(355): Mautic\\CoreBundle\\Controller\\AbstractFormController->isFormValid(Object(Symfony\\Component\\Form\\Form))
#11 /var/www/html/app/bundles/CoreBundle/Controller/CommonController.php(456): Mautic\\ReportBundle\\Controller\\ReportController->editAction('18', '')
#12 /var/www/html/vendor/symfony/http-kernel/HttpKernel.php(151): Mautic\\CoreBundle\\Controller\\CommonController->executeAction('edit', '18', 0, '')
#13 /var/www/html/vendor/symfony/http-kernel/HttpKernel.php(68): Symfony\\Component\\HttpKernel\\HttpKernel->handleRaw(Object(Symfony\\Component\\HttpFoundation\\Request), 1)
#14 /var/www/html/vendor/symfony/http-kernel/Kernel.php(200): Symfony\\Component\\HttpKernel\\HttpKernel->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)
#15 /var/www/html/app/AppKernel.php(104): Symfony\\Component\\HttpKernel\\Kernel->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)
#16 /var/www/html/app/middlewares/CORSMiddleware.php(91): AppKernel->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)
#17 /var/www/html/app/middlewares/CatchExceptionMiddleware.php(43): Mautic\\Middleware\\CORSMiddleware->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)
#18 /var/www/html/app/middlewares/Dev/IpRestrictMiddleware.php(61): Mautic\\Middleware\\CatchExceptionMiddleware->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)
#19 /var/www/html/app/middlewares/VersionCheckMiddleware.php(67): Mautic\\Middleware\\Dev\\IpRestrictMiddleware->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)
#20 /var/www/html/app/middlewares/TrustMiddleware.php(51): Mautic\\Middleware\\VersionCheckMiddleware->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)
#21 /var/www/html/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Mautic\\Middleware\\TrustMiddleware->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)
#22 /var/www/html/vendor/stack/run/src/Stack/run.php(13): Stack\\StackedHttpKernel->handle(Object(Symfony\\Component\\HttpFoundation\\Request))
#23 /var/www/html/index_dev.php(25): Stack\
un(Object(Stack\\StackedHttpKernel))
#24 {main}
"} 

RCheesley
RCheesley previously approved these changes Mar 19, 2021
@RCheesley RCheesley dismissed their stale review March 19, 2021 18:19

Found an error after approving

@domparry
Copy link

This has fixed our email error too.

@RCheesley
Copy link
Sponsor Member

https://youtu.be/-aTz-uQV9yo Here is a screencast of the error that I was seeing with the report - it was a few days ago but I am pretty sure that I checked at the time that it did not happen before the PR was applied.

@stevedrobinson
Copy link
Contributor

I was getting this error (I think it's the same) when trying to view a contacts report with a filter based on segment.

Before the PR, I got this error in the log and a 500 in the browser:
PHP message: PHP Notice: Trying to access array offset on value of type null in /path/to/mautic/app/bundles/ReportBundle/Form/Type/FilterSelectorType.php on line 80" while reading response header from upstream, client: XX.XX.XX.XX, server: my.mauticinstance.com, request: "GET /s/reports/edit/17 HTTP/2.0", upstream: "fastcgi://unix:/run/php/php7.4-fpm.sock:", host: "ma.brilliantmetrics.com", referrer: "https://my.mautic.instance/s/reports/edit/17

After applying the PR, I get this error in the log and a 500 error in the browser:
PHP message: PHP Warning: Illegal string offset 'column' in /path/to/mautic/html/app/bundles/ReportBundle/Form/Type/FilterSelectorType.php on line 89" while reading response header from upstream, client: xx.xx.xx.xx, server: my.mauticinstance.com, request: "POST /s/reports/edit/17?mauticUserLastActive=26&mauticLastNotificationId=12179 HTTP/2.0", upstream: "fastcgi://unix:/run/php/php7.4-fpm.sock:", host: "my.mauticinstance.com", referrer: "https://my.mauticinstance.com/s/reports/edit/17

Let me know if I can offer any more info and/or test again...

@mohit-rocks
Copy link
Contributor Author

mohit-rocks commented Mar 31, 2021

@stevedrobinson I tried to fix that issue in last commit bb577a0 of this PR
Can you please try to check once in MautiBox.

@RCheesley
Copy link
Sponsor Member

@mohit-rocks are you able to rebase this on features please, so it could be considered for 4.0?

@stevedrobinson
Copy link
Contributor

I retested in MautiBox and if I create a contacts report and add a filter for [Email] [is not empty] I get the following errors:

/app/bundles/ReportBundle/Form/Type/FilterSelectorType.php:89 at
/app/bundles/ReportBundle/Form/Type/FilterSelectorType.php:89 at Mautic\CoreBundle\ErrorHandler\ErrorHandler -> handleError ( '2', 'PHP Warning - Illegal string offset 'column'', '/var/app/current/code/pulls/9795/app/bundles/ReportBundle/Form/Type/FilterSelectorType.php', '89', array('event' => object(FormEvent), 'formModifier' => object(Closure), 'data' => 'l.email') )
/vendor/symfony/event-dispatcher/EventDispatcher.php:214 at Mautic\ReportBundle\Form\Type\FilterSelectorType -> Mautic\ReportBundle\Form\Type\{closure} ( object(FormEvent), 'form.pre_bind', object(EventDispatcher) )
/vendor/symfony/event-dispatcher/EventDispatcher.php:44 at Symfony\Component\EventDispatcher\EventDispatcher -> doDispatch ( array(array(object(TrimListener), 'preSubmit'), array(object(CsrfValidationListener), 'preSubmit'), object(Closure)), 'form.pre_bind', object(FormEvent) )

@mohit-rocks mohit-rocks changed the base branch from 3.3 to features May 16, 2021 04:09
@RCheesley
Copy link
Sponsor Member

Thanks for working further on this @mohit-rocks - am now able to create reports (including with segment filters) without getting errors!

Was this a 4.0 change that we have missed? If so let's make sure that we have it documented in the upgrade-4.0.md file.

@stevedrobinson any chance you could re-test?

@RCheesley RCheesley added the pending-test-confirmation PR's that require one test before they can be merged label May 16, 2021
@RCheesley RCheesley added this to the 4.0-rc milestone May 16, 2021
@sensalot
Copy link

Thanks for working further on this @mohit-rocks - am now able to create reports (including with segment filters) without getting errors!

Was this a 4.0 change that we have missed? If so let's make sure that we have it documented in the upgrade-4.0.md file.

@stevedrobinson any chance you could re-test?

@RCheesley Does this mean we can't create new reports / view old ones untill 4.0?

@mautibot
Copy link

This pull request has been mentioned on Mautic Community Forums. There might be relevant details there:

https://forum.mautic.org/t/after-update-tot-3-2-5-http-500-when-creating-a-new-report-and-when-opening-an-old-one/19517/2

@sensalot
Copy link

sensalot commented May 19, 2021

We added the two files to Mautic 3.2.5.
Editing of report became possible. Applying seems to work. A save and close seems to hang. View of many report is not possible. New created report: viewing is not possible.
Tested with Chrome, Firefox Edge. 2 devices in different locations.

Updgrading to 3.3.3. fixed the problem

Copy link
Contributor

@shinde-rahul shinde-rahul left a comment

Choose a reason for hiding this comment

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

Code changes looks good to me.

@RCheesley RCheesley added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed pending-test-confirmation PR's that require one test before they can be merged labels May 21, 2021
@RCheesley RCheesley merged commit cb379d3 into mautic:features May 21, 2021
@RCheesley
Copy link
Sponsor Member

@all-contributors please add @mohit-rocks for code

@allcontributors
Copy link
Contributor

@RCheesley

I've put up a pull request to add @mohit-rocks! 🎉

@RCheesley
Copy link
Sponsor Member

@all-contributors please add @domparry for userTesting

@allcontributors
Copy link
Contributor

@RCheesley

I've put up a pull request to add @domparry! 🎉

@RCheesley
Copy link
Sponsor Member

@all-contributors please add @sensalot for userTesting

@allcontributors
Copy link
Contributor

@RCheesley

I've put up a pull request to add @sensalot! 🎉

@RCheesley
Copy link
Sponsor Member

@all-contributors please add @shinde-rahul for review

@allcontributors
Copy link
Contributor

@RCheesley

I've put up a pull request to add @shinde-rahul! 🎉

@RCheesley RCheesley modified the milestones: 4.0-rc, 4.0-beta May 25, 2021
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 cla-signed The PR contributors have signed the contributors agreement dynamic-content email Anything related to email essential This must be done to close the milestone ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged regression A bug that broke something in the last release segments Anything related to segments T1 Low difficulty to fix (issue) or test (PR)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple areas of Mautic throw a 500 error
7 participants