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

Initial implementation #1

Draft
wants to merge 12 commits into
base: develop
Choose a base branch
from
Draft

Initial implementation #1

wants to merge 12 commits into from

Conversation

mecha
Copy link
Member

@mecha mecha commented Mar 16, 2021

  • Add analysis tools
  • Add manipulation tools
  • Add graphing tools

@mecha mecha requested a review from XedinUnknown March 16, 2021 22:19
@mecha mecha self-assigned this Mar 16, 2021
Copy link
Member

@XedinUnknown XedinUnknown left a comment

Choose a reason for hiding this comment

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

Superb. Not much more I can say. Can I merge this?

Comment on lines -74 to +76
<configuration_option name="xdebug.remote_host" value="host.docker.internal" />
<configuration_option name="xdebug.remote_host" value="172.17.0.1" />
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK it should be host.docker.internal, which is mapped to whatever IP via the docker-compose.yml.

Copy link
Member Author

Choose a reason for hiding this comment

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

host.docker.internal is Windows-specific, while AFAIK 172.17.0.1 is cross-platform.

Copy link
Member

Choose a reason for hiding this comment

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

host.docker.internal is Windows specific with what Docker does automatically. But in php-project setup we simply map the configured IP to hosts.docker.internal, and so it will exist either way. If you can find where 172.17.0.1 is being documented to be a cross-platform way to access the host, I can put it into .env.example, and it should work for everyone 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's continue this discussion in a more appropriate channel.

Comment on lines +14 to +15
* @param array<callable|ServiceInterface> $factories The list of factories to analyze.
* @param array<callable|ServiceInterface> $extensions The list of extensions to analyze.
Copy link
Member

Choose a reason for hiding this comment

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

Can these be iterable instead of array?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure. It's been a while since I wrote this. I'll get back to you on this.

Copy link
Member Author

Choose a reason for hiding this comment

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

They can't be iterable because the desired type is, in actuality, array<string, callable|ServiceInterface>.

@mecha
Copy link
Member Author

mecha commented Jun 24, 2021

Update: I have a ton of unpushed and uncommitted code in my local copy. Most of it has to do with the addition of a graph traverser, and rewrites of the existing analyzers to implement the visitor interface.

The traverser would be very beneficial change for projects with larger service graphs, since the traverser only iterates the graph once whereas the current design has each analyzer iterate the graph for its own consumption.

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.

2 participants