Skip to content

Conversation

@enumag
Copy link
Contributor

@enumag enumag commented May 6, 2021

Closes #472

@enumag
Copy link
Contributor Author

enumag commented May 6, 2021

@dg The failure is not caused by my changes.

@dg
Copy link
Member

dg commented May 6, 2021

Can you add a test?

@enumag
Copy link
Contributor Author

enumag commented May 6, 2021

The test would of course require ext-ds to be installed. I'm not sure how to add it on your GitHub Actions setup.

@dg
Copy link
Member

dg commented May 6, 2021

In this way: https://github.com/nette/latte/blob/master/.github/workflows/tests.yml#L20

(Probably extensions: ds)

@enumag
Copy link
Contributor Author

enumag commented May 6, 2021

@dg How do I run the tests locally? I tried composer tester but it fails with

   Error: Interface 'JsonSerializable' not found

I have no idea why. I'm using json extension every day and in my code it always exists. So I'm very confused. Can you fix composer tester so that it would work?

@enumag enumag force-pushed the feature/ext-ds branch 2 times, most recently from 4a96a60 to d1cf1f3 Compare May 6, 2021 13:48
@enumag enumag force-pushed the feature/ext-ds branch from d1cf1f3 to e1dbcd6 Compare May 6, 2021 13:50
@milo
Copy link
Member

milo commented May 6, 2021

vendor/bin/tester -C tests should do the job

@enumag
Copy link
Contributor Author

enumag commented May 10, 2021

@milo thx for the advice, I fixed composer.json to use this option too.

@dg
Copy link
Member

dg commented May 10, 2021

ad JsonSerializable: what operating system are you using?

@enumag
Copy link
Contributor Author

enumag commented May 10, 2021

@dg Ubuntu but I don't think it's important. The issue is that nette/tester isn't using the system's default php.ini without the -C option so it didn't load any extensions. It is very counter-intuitive for someone not experienced with nette/tester.

If there are reasons to not load the default php.ini in some scenarios then in my opinion it should work the other way around - use system's php.ini by default with an option to turn it off.

@dg
Copy link
Member

dg commented May 10, 2021

The point is, it works for me on Windows and I had no idea there is such a problem.

This has to be solved with a custom php.ini, I'll fix it so please don't change the composer.json.

@enumag
Copy link
Contributor Author

enumag commented May 10, 2021

Alright, I removed the composer.json commit. Can you review the feature I'm adding? :-)

@dg
Copy link
Member

dg commented May 10, 2021

Looks good, thanks

@dg dg changed the title Add ext-ds support Dumper: added ext-ds support May 10, 2021
@dg dg merged commit 9585ab3 into nette:master May 10, 2021
dg pushed a commit that referenced this pull request May 10, 2021
dg pushed a commit that referenced this pull request May 10, 2021
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.

ext-ds support

3 participants