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

Add PHP implementation. #11

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

syntaxseed
Copy link

A basic PHP implementation.

  • No sessions or saving.
  • Copied HTML from Ruby version.
  • PHP v 7.3+.

@gypsydave5
Copy link
Owner

Hi @syntaxseed - my apologies for not getting back to you sooner! This PR looks great - I've just run it locally and it seems to work like a dream.

However it's failing the acceptance tests (boo!). I'm no PHP developer so I'm not sure I'm going to be best placed to fix it, but I'll have a poke around to see if I can spot the issue(s). If you've got time please do the same.

Thanks for helping out!

@syntaxseed
Copy link
Author

Hmm... unfortunately I don't have a Ruby environment in which to run the tests.

If it's just a few failing perhaps you could paste the test output here & I could try to figure out what's failing from reading the tests?

@gypsydave5
Copy link
Owner

Hey - sorry for letting this go so fallow for so long. I've been able to take a proper look at it this evening and I think I can see the problem (although I can't fix it as I'm clueless about PHP)

At least one issue is that the acceptance tests are expecting that a post to / will add a new todo; you app currently only accepts a post to /add to add a new todo.

This is my fault as I've not documented the API explicitly (outside of the acceptance tests). I'll try and do that in the next few days.

@syntaxseed - do you think you could update your app to take the new todo POSTs to /?

Thanks!

@syntaxseed
Copy link
Author

Will fix!

@syntaxseed
Copy link
Author

@gypsydave5 fixed the route to use / instead of /add for adding a new todo.

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.

3 participants