Skip to content

Dumper: added ext-ds support#484

Merged
dg merged 2 commits into
nette:masterfrom
enumag:feature/ext-ds
May 10, 2021
Merged

Dumper: added ext-ds support#484
dg merged 2 commits into
nette:masterfrom
enumag:feature/ext-ds

Conversation

@enumag

@enumag enumag commented May 6, 2021

Copy link
Copy Markdown
Contributor

Closes #472

@enumag

enumag commented May 6, 2021

Copy link
Copy Markdown
Contributor Author

@dg The failure is not caused by my changes.

@dg

dg commented May 6, 2021

Copy link
Copy Markdown
Member

Can you add a test?

@enumag

enumag commented May 6, 2021

Copy link
Copy Markdown
Contributor Author

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

dg commented May 6, 2021

Copy link
Copy Markdown
Member

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

(Probably extensions: ds)

@enumag

enumag commented May 6, 2021

Copy link
Copy Markdown
Contributor Author

@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

milo commented May 6, 2021

Copy link
Copy Markdown
Member

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

@enumag

enumag commented May 10, 2021

Copy link
Copy Markdown
Contributor Author

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

@dg

dg commented May 10, 2021

Copy link
Copy Markdown
Member

ad JsonSerializable: what operating system are you using?

@enumag

enumag commented May 10, 2021

Copy link
Copy Markdown
Contributor Author

@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

dg commented May 10, 2021

Copy link
Copy Markdown
Member

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

enumag commented May 10, 2021

Copy link
Copy Markdown
Contributor Author

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

@dg

dg commented May 10, 2021

Copy link
Copy Markdown
Member

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