-
Notifications
You must be signed in to change notification settings - Fork 16
Controller Stub #14
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
Controller Stub #14
Conversation
…ll be used for testing
b646f70 to
38c7032
Compare
…to local PHP server
src/Controller.php
Outdated
| * | ||
| * @return \Zend\Http\Response | ||
| */ | ||
| public function run(): \Zend\Http\Response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thought was this would be a good use case for __invoke() but I'm not absolutely stuck on that idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made this change assuming this would be the only public method. I'm not a fan of calling $controller() in bin/upward, if you have a better naming convention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷♂️ Naming stuff is one of the two hard things in programming.
Calling this class a "Controller" was the closest concept I could think of for it's purpose, but I welcome any better ideas.
…tion - Pass configuration to controller captured from environment variable - Fix package.json warnings
bin/upward
Outdated
| if (!$upwardConfig) { | ||
| // this appears to violate the spec, since we can't kill the server until we receive a request | ||
| // will leave here for right now until we can discuss with Z | ||
| posix_kill(posix_getpid(), SIGKILL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're sending a KILL signal back to ourselves? That seems bit like overkill.
Seriously though, I think the exit(1) below should be just as effective and a lot cleaner than this line. Either way, I get the feeling that having both is redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think this needs discussed further. Technically the server should never start if config is missing or malformed, but given the current logic, that validation would have to be in that shell script that starts the server.
This doesn't conform to the spec now anyway, so I'll just remove the SIGKILL.
…ctionality with James - Refactor after renaming of DefinitionIterator - Write unit test for Controller
Added 7.x version of symfony yaml in the dependency list
Begin work stubbing out the entry point for this app