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

Dumper: added ext-ds support #484

Merged
merged 2 commits into from
May 10, 2021
Merged

Dumper: added ext-ds support #484

merged 2 commits into from
May 10, 2021

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
@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