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

Ready for review: Migration to Symfony 3 #37

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

Conversation

CoalaJoe
Copy link
Contributor

Created this PR for you to keep track of what I am doing. Please don't be afraid to ask!
I will help where I can and add documenation where needed.

.editorconfig Outdated
-
-; Unix-style newlines
-[*]
-end_of_line = LF
Copy link
Member

Choose a reason for hiding this comment

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

Looks like there's a broken diff/merge here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it

@CoalaJoe CoalaJoe changed the title DO NOT MERGE: Migration to Symfony 3 Ready for review: Migration to Symfony 3 Feb 21, 2017
@CoalaJoe
Copy link
Contributor Author

Can you try to check the code, for compatibility with the current state?

@RobLoach
Copy link
Member

Wow, nicely done!

Getting the following error when attempting to create the database:

Could not create database /home/rob/Documents/libretro-netplay-registry/app/../var/data/data.sqlite for connection named default
An exception occured in driver: SQLSTATE[HY000] [14] unable to open database file

Know how to fix the database path?

@CoalaJoe
Copy link
Contributor Author

Ah. Git does not like empty folders. When you create the data-folder in the var-folder, it should work. I'll add a .gitkeep to that directory, so that this won't happen anymore.

@RobLoach
Copy link
Member

RobLoach commented Feb 22, 2017

Git does not like empty folders.

One common convention is to have a .gitkeep file. Oh, you said that. lol... I should read all comment before responding.

@CoalaJoe
Copy link
Contributor Author

Tonight I'll write tests for the other APIs. So we have 100% coverage.

@RobLoach
Copy link
Member

Very cool! I'll have another chance to test more tonight.

@CoalaJoe
Copy link
Contributor Author

@RobLoach Thanks :) Any feedback on #37 (comment)

@CoalaJoe
Copy link
Contributor Author

CoalaJoe commented Feb 28, 2017

I got another idea.
1 entry per username & ip combo.
5 entries per ip.

The old entries will be replaced.
btw @fr500 is there any plan on adding a fixed username per player?

…miting entries per ip or ip username combo
@CoalaJoe
Copy link
Contributor Author

The latest commit is ready for review. But needs tests for lifecycle callback and entrymanagementlistener so don't merge just yet. ;)

@CoalaJoe
Copy link
Contributor Author

CoalaJoe commented Mar 1, 2017

It is ready for a review.

@CoalaJoe
Copy link
Contributor Author

CoalaJoe commented Mar 3, 2017

@RobLoach @fr500 Any feedback on when you have time to review it?

@CoalaJoe
Copy link
Contributor Author

CoalaJoe commented Mar 6, 2017

Anything?

@RobLoach
Copy link
Member

RobLoach commented Mar 6, 2017

Looks great from my perspective. Tested it a bit ago and entries were added correctly. Even set up an Apache local host and the routes picked up correctly.

I'd be open to merging! @fr500, think we're ready? We could test this on a different branch on lobby.libretro.com if you want to make sure it works on the server beforehand.

@andres-asm
Copy link
Contributor

andres-asm commented Mar 6, 2017 via email

@CoalaJoe
Copy link
Contributor Author

CoalaJoe commented Mar 6, 2017

@fr500 cool...
So what now?

@CoalaJoe
Copy link
Contributor Author

CoalaJoe commented Mar 7, 2017

Seems like @bparker06 didn't do anything in his fork.
@fr500 what do we do?

@RobLoach
Copy link
Member

RobLoach commented Mar 7, 2017

@CoalaJoe
Copy link
Contributor Author

CoalaJoe commented Mar 7, 2017

I am closing this PR due to the current state of this repository.
It's a pitty that so much effort has been wasted, but there is nothing I can do.

@CoalaJoe CoalaJoe closed this Mar 7, 2017
@andres-asm
Copy link
Contributor

it's not really our fault either, @RobLoach at least was aware that this thing was coded on a whim, on a weekend were I decided we were gonna have a netplay quick connect feature, he was available and he helped me with the PHP side of things.

It was developed as a sort of proof of concept and nothing else.

@RobLoach
Copy link
Member

RobLoach commented Mar 7, 2017

@fr500 Is there anything holding us back from merging this one though? It's a lot better then what's on lobby.libretro.com right now. Even though newlobby is coming, this will keep us going until it's switched with the Python one.

Also, this has tests 😉

@RobLoach RobLoach reopened this Mar 7, 2017
@andres-asm
Copy link
Contributor

We migrated already... I don't know really, this is off my hands now

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.

None yet

3 participants