Loris: Update singleton() declarations to use the Factory class

[johnsaigle]

Occurrences like:

$DB =& \Database::singleton();

should be changed to:

$DB = \NDB_Factory::singleton()->database();

Going through the Factory class allows for more robust testing down the line.

[AlexandraLivadas]

Here's a text file of all the files that use it and with what frequency:

dbSingletonListWithFrequency.pdf

Which of these do you think would be good to start with @johnsaigle @driusan ? I can start with the Utility class if you'd like because I already have unit tests written for it.

[driusan]

I think you should start with finalizing your currently existing PRs and trying to get them merged.. after that, I don't think it matters much, either whichever one you think is easiest, or whichever one the current singleton() behaviour is blocking you the most from writing tests that you want to write.

The classes that are probably highest value to have tests for in that list are User, Candidate, TimePoint, Utility, and NDB_Menu_Filter (in no particular order.)

[AlexandraLivadas]

Ok. Should I finalize the PRs that have tests written differently because of the Database::singleton() problem though?

[johnsaigle]

If it simplifies the testing process for you, I think it would be fine to e.g. change all the singleton problems in Config in the same PR where you're adding tests for Config

[driusan]

It's up to you if you want to go back and change those PRs to use the factory or keep them as-is, if the current tests are already working.

[AlexandraLivadas]

The tests are working but there would be less clutter if I were to change to the factory way. I can do it for my Utility PR and see how it goes? I'll ask @kongtiaowang as well!

[AlexandraLivadas]

Occurences like this:

$user = User::singleton();

should also be changed, to this:

$user = $factory->user();

This also makes unit testing easier.

@johnsaigle Should I make a new issue for this?

[johnsaigle]

Yep that'd be great!

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[johnsaigle]

I think this is another one that we can add to the "Code Review Checklist" document rather than keep as an open-ended issue.