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

App::encodeJson(): catch case that preg_replace_callback fails #1540

Closed
PhilippGrashoff opened this issue Nov 17, 2020 · 20 comments · Fixed by #1573
Closed

App::encodeJson(): catch case that preg_replace_callback fails #1540

PhilippGrashoff opened this issue Nov 17, 2020 · 20 comments · Fixed by #1573

Comments

@PhilippGrashoff
Copy link
Collaborator

Hi,

I recently managed (don't know how, but not too important) to get this error message:

Bildschirmfoto von 2020-11-17 21-16-16

Logic behind this seems that preg_replace_callback() returns null on failure. We should take care of this.
I'd go for this, WDYT?


    public function encodeJson($data, bool $forceObject = false): string
    {
        $options = JSON_UNESCAPED_SLASHES | JSON_PRESERVE_ZERO_FRACTION | JSON_UNESCAPED_UNICODE | JSON_PRETTY_PRINT;
        if ($forceObject) {
            $options |= JSON_FORCE_OBJECT;
        }

        $json = json_encode($data, $options | JSON_THROW_ON_ERROR, 512);

        // IMPORTANT: always convert large integers to string, otherwise numbers can be rounded by JS
        // replace large JSON integers only, do not replace anything in JSON/JS strings
        $jsonLargeNumbers = preg_replace_callback('~(?:"(?:[^"\\\\]+|\\\\.)*")?+\K|(?:\'(?:[^\'\\\\]+|\\\\.)*\')?+\K|(?:^|[{\[,:])'
            . '[ \n\r\t]*\K-?[1-9]\d{15,}(?=[ \n\r\t]*(?:$|[}\],:]))~s', function ($matches) {
                if ($matches[0] === '' || abs((int) $matches[0]) < (2 ** 53)) {
                    return $matches[0];
                }

                return '"' . $matches[0] . '"';
            }, $json);
        
        if($jsonLargeNumbers == null) {
            return $json;
        }
       
        return $jsonLargeNumbers;
    }
@PhilippGrashoff
Copy link
Collaborator Author

Hi,

I just stumbled across the second situation in which preg_replace_callback does not work and returns null...

@bedengler
Copy link
Contributor

I had the same issue!
And then I just came across something: I've imported some datasets into the table yesterday. Before that, everything worked.
Now I've checked the columns of the table. The affected column/field has a tinyint(1) configured, BUT the default value in the table for this field is defined (by Sequel Pro) as "NULL" (even though it's a boolean field). Therefore all entries had "NULL" in this field.
I corrected that now manually to "0" and set the Standard value to 0 and this seems to have done the trick. I just don't know currently if this is "allowed", if I want to go with mysql standards...

Deactivating the preg_replace... etc. part also worked!
But it seems connected to the database and the value that's coming back from it...

In my case, this wasn't working any more with radio buttons:

$extra->addControl('field1', [
        \atk4\ui\Form\Control\Radio::class,
      ],
      [
        'default' => 0,
        'values' => [
          0 => 'Direct-Client',
          1 => 'Agency',
        ],
      ]);

After changing the field in the table, everything worked again...

@bedengler
Copy link
Contributor

bedengler commented Nov 25, 2020

Another insight that may help in fixing.
This WORKS:

$this->hasOne('contact_Parent_ID', [
          new Model($this->persistence),
          'their_field' => 'contact_ID',
          'our_field' => 'contact_Parent_ID',
          'ui' => [
            'form' => [
                'width' => 'six',
            ]
          ],
          'type' => 'integer',
        ]);

This DOESN'T WORK:

$this->hasOne('contact_Parent_ID', [
          new Model($this->persistence),
          'their_field' => 'contact_ID',
          'our_field' => 'contact_Parent_ID',
          'ui' => [
            'form' => [
                \atk4\ui\Form\Control\Dropdown::class,
                'width' => 'six',
            ]
          ],
          'type' => 'integer',
        ]);

In my case, the dropdown gets generated automatically, so there's no need of adding it in the form manually.

There's a relation defined in the database / table. I assume that's why the dropdown gets generated automatically (which is pretty awesome 😉 )

@mvorisek
Copy link
Member

@PhilippGrashoff I will fix if needed immediatelly, post minimal steps to reproduce, as best as PR with test

@mvorisek mvorisek self-assigned this Nov 25, 2020
@bedengler
Copy link
Contributor

It's hard for me to put out a test case as I'd have to post the whole database (because of the connections) and the database is filled with real data, no test data.

Therefore I made a dump of the structure, I hope this helps you!?

I attached a ZIP containing:

  1. The database structure
  2. Sample code of the model and the form that is related to the issue
  3. The JSON output from App::jsonEncode (this seems to be always the same - no matter if it works or doesn't work)

The difference is in the "hasOne".
As soon as I'm adding "\atk4\ui\Form\Control\Dropdown::class," there, I'm getting the error.

If I remove it and there's no relation in the table, the field is just a text field.
If I'm setting up the relation then, the field becomes a dropdown. That's how it should be...

Archive.zip

@mvorisek
Copy link
Member

mvorisek commented Nov 26, 2020

@bedengler just var_dump args of App::jsonEncode and verify the issue with that input. I can fix promtly then. Maybe @PhilippGrashoff can help with the test case as he opened this issue.

@mvorisek
Copy link
Member

closing as no reaction

@mvorisek mvorisek removed their assignment Dec 14, 2020
@PhilippGrashoff
Copy link
Collaborator Author

@mvorisek please leave this open. This issue exists, and I will provide a test case when I have time to do so.

@mvorisek
Copy link
Member

@PhilippGrashoff feel free to provide, I will investigate then :)

@IvanTonchev
Copy link

I stumbled across this issue while working on a more complex JsModal+VirtualPage.
In the end i concluded that it's a preg_replace_callback() related limitation issue which has nothing to do with my code or data. After reaching certain limit, adding any control to the form was triggering the null result. I can't say whether the regex in encodeJson() needs improvement or it's a pure preg_replace_callback() issue.
See here or here
I had to override the encodeJson() method and removed the preg_replace_callback() bit.

@PhilippGrashoff
Copy link
Collaborator Author

Hi Ivan,

@mvorisek is still lacking demo data to debug this problem. can you get the parameters passed to App::encodeJson() in case of a failure? That would be great. I just dont find time for this at the moment.

Best regards

@mvorisek
Copy link
Member

@IvanTonchev this IS A MAJOR ISSUE then - the regex is designed wrongly

please var_export input data for which the preg_replace_callback fails, ideally make this https://3v4l.org/LOhUU code failing

@bedengler
Copy link
Contributor

Did you investigate the files I've uploaded? They also contain the JSON output...maybe that helps!?

@mvorisek
Copy link
Member

@bedengler please make this https://3v4l.org/LOhUU code failing with your data

@IvanTonchev
Copy link

var_export.txt
https://3v4l.org/LOhUU failed with '413 Request Entity Too Large'

@mvorisek
Copy link
Member

@IvanTonchev
Copy link

@mvorisek it's a step closer towards resolution ;)

@mvorisek
Copy link
Member

Guys, please verify with real data.

@IvanTonchev
Copy link

@mvorisek i can confirm that the fixed encodeJson() regex works in my setting (atk4/ui 2.3.1, PHP 7.3.19).

@mvorisek
Copy link
Member

thanks!

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

Successfully merging a pull request may close this issue.

4 participants