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

[VarDumper] Add Dump object to configure dumping options #17290

Closed
wants to merge 1 commit into from

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Jan 7, 2016

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #17103, #12073, #18479, #16618
License MIT
Doc PR -

The goal of this PR is to allow devs to configure all dumping options.
The targeted usage would be something like dump($foo)->to('php://stderr')->as('text')->withMaxDepth(3)->die();

Steps:

  • list all existing options that configure dumps
  • provide a builder that aggregates all the dumping options in one fluid class
  • configure with a modeline in an ENV var / HTTP header / ini setting
  • validate withTitle() usefulness, implement
  • implement withTrace() and enable it by default when no var is provided
  • integrate the modeline with dump()/VarDumper::dump
  • integrate the modeline with the profiler
  • add tests

return 'cli' === PHP_SAPI ? 'cli' : 'html';
}

protected function getDumper($format, $output)
Copy link
Member

Choose a reason for hiding this comment

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

does it need to be protected ?

Copy link
Member Author

Choose a reason for hiding this comment

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

to make the class SOLID, especially the "O": it opens to specializations by "di-via-getter-specialization"

Copy link
Member

Choose a reason for hiding this comment

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

well, this object is a kind-of value object. Using inheritance as an extension point looks weird to me (and inheritance is the most painful way to open for extension as far as BC is concerned)

Copy link
Member Author

Choose a reason for hiding this comment

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

To me, this object is way more a builder than a value object; builders are specialized, and looking at the feedback I see here and here, this one will be specialized... Thus, I'd want it to be possible from the beginning, by inheritance or something better if possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not to inject Dumper into constructor then? You still can initialize default dumper with static factory method if you really want it.

Copy link
Member Author

Choose a reason for hiding this comment

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

the returned dumper depends on two parameters, we can't inject only one. We could inject a factory thought, but that looks overkill to me for now...

@stof
Copy link
Member

stof commented Jan 7, 2016

How would this integrate with the profiler ?

'withRefHandles' => array(true),
'withCasters' => array(array(), false),
'withFilter' => array(0),
);
Copy link
Contributor

Choose a reason for hiding this comment

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

why an array like this rather than a bunch of properties ?

Copy link
Member Author

Choose a reason for hiding this comment

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

because I use __call (same answer for your other comments)

@nicolas-grekas
Copy link
Member Author

How would this integrate with the profiler ?

I don't know yet

@stof
Copy link
Member

stof commented Jan 7, 2016

I don't know yet

This should be figured out. If it cannot send dumps to the profiler, it is not the right API IMO

/**
* @author Nicolas Grekas <p@tchwork.com>
*/
class Dump2
Copy link
Member Author

Choose a reason for hiding this comment

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

@stof I added this Dump2 implementation so that we can compare with and without __call, wdyt?

@nicolas-grekas nicolas-grekas force-pushed the dump-buider branch 2 times, most recently from ad1acaa to 61dd58a Compare January 14, 2016 09:16
fabpot added a commit that referenced this pull request Apr 15, 2016
… Doctrine Cache (nicolas-grekas)

This PR was merged into the 3.1-dev branch.

Discussion
----------

[Cache] Add DoctrineProvider, for using PSR-6 pools in Doctrine Cache

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

This would allow cache pool configuration as usual (see #17290) before injecting the resulting doctrine cache provider anywhere such an interface is required.

Commits
-------

5d256dd [Cache] Add DoctrineProvider, for using PSR-6 pools in Doctrine Cache
@fabpot
Copy link
Member

fabpot commented Jul 1, 2016

@nicolas-grekas Are we far away from something that works?

@nicolas-grekas
Copy link
Member Author

I'm not close to finishing but I make slow progress :)

@nicolas-grekas
Copy link
Member Author

Closing until it's ready again

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

Successfully merging this pull request may close these issues.

None yet

7 participants