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

Remove duplicate call to install() method #533

Merged
merged 3 commits into from May 15, 2014
Merged

Remove duplicate call to install() method #533

merged 3 commits into from May 15, 2014

Conversation

fjarrett
Copy link
Contributor

Resolves #528

This removes a second call of install() that is already handled earlier by the verify_database_present() method.

Also removes duplicate use of the wp_stream_no_tables filter in install() that is also handled beforehand by verify_database_present().

I've tested in single site and multisite. Fresh installs and DB upgrade routines are working in both.

@shadyvb @lukecarbis Please review

@lukecarbis
Copy link
Contributor

I don't like that verify_database_present() does more than you would expect it to by reading it's name. It should be verify_database_present_and_install() (which is a bit ugly).

But then, I prefer the idea that functions do one thing only.

Would this approach work instead?

  1. Add install() to the init hook during __construct
  2. Add verify_database_present() to the wp_stream_no_tables hook during __construct
  3. Remove the apply_filters( 'wp_stream_no_tables' ) conditional from the verify_database_present() function
  4. Add the apply_filters( 'wp_stream_no_tables' ) conditional to the install function (before installing)
  5. Remove the self::install() call made by verify_database_present()

This approach says "install, but do a check to see if the database exists before you get too far". Rather than, "check to see if the database exists, and if it does, install". The focus of our logic should be on installing, not verifying, IMO.

Or have I missed something?

@fjarrett
Copy link
Contributor Author

@lukecarbis Nice idea.

@fjarrett
Copy link
Contributor Author

@lukecarbis Actually I don't see how we can use wp_stream_no_tables inside the __construct because it's just a filter used to pass a boolean value. But I'll try a different approach based on this concept.

@lukecarbis
Copy link
Contributor

👍 Much better.

lukecarbis added a commit that referenced this pull request May 15, 2014
Remove duplicate call to install() method
@lukecarbis lukecarbis merged commit c7f1ed0 into develop May 15, 2014
@lukecarbis lukecarbis deleted the issue-528 branch May 15, 2014 23:09
@fjarrett fjarrett restored the issue-528 branch May 15, 2014 23:23
@fjarrett fjarrett deleted the issue-528 branch May 15, 2014 23:23
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.

Stream runs install routine twice
2 participants