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

Fix for multisite usage #16

Merged
merged 13 commits into from Jul 25, 2016

Conversation

Projects
None yet
2 participants
@afragen
Copy link
Collaborator

commented Jul 17, 2016

Fix for #4

@afragen afragen changed the title Fix for multisite usage #4 Fix for multisite usage Jul 17, 2016

@afragen

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 17, 2016

Not quite ready yet. Settings not getting saved in multisite.

@afragen

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 17, 2016

OK, this should actually save the settings. 😝

@senlin

This comment has been minimized.

Copy link
Owner

commented Jul 17, 2016

Hi Andy, thank you so much for diving into this and fixing it.

I have a question regarding your PR:
Should the uninstall file be adjusted for Multisite or can it remain as is?

@afragen

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 18, 2016

Hey Piet! My pleasure, just installed the plugin and saw the issue. Had some free time. 😉

I didn't change a thing for the options so the uninstall is fine. I did have a question re: options. Why didn't you save as an array instead of all individual items?

Saving as an array would eliminate the foreach loop in the network settings and make uninstall simpler.

For the uninstall it usually just have a bunch of delete_option() and delete_site_options() calls. I don't usually delete directly from the database.

As long as the network settings are suppose to be same as all subsite settings then it's ok. The way it's set up the main site will have the same settings as network. I can fix it so network settings override and subsite settings aren't allowed. But that would be your choice.

We can discuss that in another issue if you want.

@afragen afragen referenced this pull request Jul 18, 2016

Closed

Move options to array #17

@afragen

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 18, 2016

It's way too difficult at the moment to figure out how to switch the options to an array. I'm gonna pass.

@afragen

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 18, 2016

Not ready for merge. I thought of a way to support multisite options better. I'll let you know when PR is ready. It will be better for subsites.

@ghost

This comment has been minimized.

Copy link

commented Jul 18, 2016

Thanks for all your work Andy!
Sent from my Xiaomi
On Andy Fragen notifications@github.com, Jul 18, 2016 8:21 AM wrote:Not ready for merge. I thought of a way to support multisite options better. I'll let you know when PR is ready. It will be better for subsites.

—You are receiving this because you are subscribed to this thread.Reply to this email directly, view it on GitHub, or mute the thread.

afragen added some commits Jul 18, 2016

final fixes for multisite compatability
use update_site_option not update_option
more fixes for multisite compatability
I forgot these in previous commit
@afragen

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 18, 2016

Hey Piet! I think I'm finished. I'm using PhpStorm and there were a number of tabs/spaces formatting stuff that I left alone. If you want me to push that too just let me know.

So here's what the PR does.

  1. Full multisite compatibility, using update_site_option() instead of update_option()
  2. In multisite the Settings only show under Network Admin. It doesn't make sense for individual site admins to be able to override the network admin settings.
  3. In single site it just works. There might be a need for re-saving settings in the switch to update_site_option/get_site_option from update_option/get_option.
  4. I added a function get_settings_as_array() to combine all the get_site_options() calls.
  5. The options naming is unchanged so the uninstall.php should work fine.
  6. I fixed up the DocBlocks, some of the new functions have @since 2.x since I don't know what version number you'll use I left it that way. I don't think the changes are breaking changes but saved settings might be lost.

Let me know if you have any other questions.

@senlin

This comment has been minimized.

Copy link
Owner

commented Jul 18, 2016

Hi Andy,
Thanks so much for your hard work!
I noticed that in some instances you have basically rewritten the code; examples are getting rid of if/else and replacing them with the colon "thingy" (for lack of a better expression). Your code is probably cleaner, but for me much less readable.
I am a bit worried that by merging your PR and when encountering trouble in the future I will not be able to fix issues anymore.

I will of course need to test your code, so I will find some time towards the end of the week to do that.

I have not heard of PhpStorm, is that to clean up whitespace?

Anyways, again many thanks for all your inout and code changes, I will keep you posted!

Cheers,
Piet

revert ternary operators for readabiltiy
I left the ternary operators in place commented out so you can see how they function.
@afragen

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 18, 2016

Hey Piet,

The "colon thingy" is called a ternary operator. It's a shorthand for an if/then/else statement.

I reverted all the ternary operators but left them in place, commented out, so you can see how they work.

PhpStorm is an IDE ( Integrated Development Environment ). There are many around, its just the one I use. PhpStorm has builtin integration with WordPress Coding Standards

You can usually use PhpStorm Early Access Program for free or a 30-day trial.

PhpStorm has lots of time saving things one is a Reformat Code menu item.

Take your time reviewing the PR and ask questions and you're welcome. 😉

when running on multisite only allow network activation
Avoids potential issues if not network active and active on subsite. As active on subsite will in effect make it network active.
@afragen

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 25, 2016

Hey Piet! Any questions I can answer?

That last commit makes it so the plugin only activates under network admin on multisite. It solves the issue of individual site admins being able to activate. Makes more sense as settings in this plugin are meant for network admin or single site admin.

@senlin senlin merged commit a932030 into senlin:master Jul 25, 2016

@senlin

This comment has been minimized.

Copy link
Owner

commented Jul 25, 2016

Hey Andy,
Everything looks great really! The only thing I'm worried about is what I already shared with you and that is if in the future something breaks regarding multisite, I will probably not be the right person to solve things.
Also because the coding you used, yours is probably better and clear, but I understand mine much better and it therefore takes much more time to troubleshoot in the future.

@senlin

This comment has been minimized.

Copy link
Owner

commented Jul 25, 2016

Anyways, I have merged your hard work and will soon update the version of the WP.org Repo. I need to do some checks first as Yoast has released a new version and who knows what bloat has been added this time? Also I always hold back a week or two for the usual and infamous x.x.y releases that follow after a Yoast update...

@senlin

This comment has been minimized.

Copy link
Owner

commented Jul 25, 2016

btw, I never knew of the "Network: true" line in the plugin header, that is a golden tip!

@afragen

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 26, 2016

Hey Piet! There shouldn't be any real problems with the multisite code and I removed the ternary operations and used the if/then/else leaving the ternary operations as reference.

Besides, I'm not going anywhere if there are issues. Just ping me. 😉

If you ever have questions about why I coded something the way I did ask. Unless something in core changes with respect to how multisite saves options, etc, the code should continue to work as I try to use core functions/hooks where I can.

You've done a great job and I'm sure you'll find more ways to improve and refactor the code.

@afragen afragen deleted the afragen:multisite branch Jul 26, 2016

@afragen

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 26, 2016

The biggest change I made was to put all the settings into an array and call the options from that created array, otherwise it's all just multisite stuff and I tried to used your variables when I named settings. Anyway, like I said, just ask. 😉

senlin pushed a commit that referenced this pull request Aug 13, 2016

Piet Bos
commit v2.4.0
* release date 2016.08.13
* with a BIG THANK YOU to [Andy Fragen](https://github.com/afragen) who
made the plugin fully Multisite compatible and therewith we have been
able to finally close [this
issue](#4).
* Andy also cleaned up some misc spacing and refactored part of the
code for it to work smoother. People interested can see the full
[PR](#16).
* tested with WP version 4.6

senlin pushed a commit that referenced this pull request Aug 13, 2016

Piet Bos
commit v2.4.0
* release date 2016.08.13
* with a BIG THANK YOU to [Andy Fragen](https://github.com/afragen) who
made the plugin fully Multisite compatible and therewith we have been
able to finally close [this
issue](#4).
* Andy also cleaned up some misc spacing and refactored part of the
code for it to work smoother. People interested can see the full
[PR](#16).
* tested with WP version 4.6
@senlin

This comment has been minimized.

Copy link
Owner

commented Aug 13, 2016

Hi Andy @afragen, a short message to let you know that I just committed the 2.4.0 release that contains your work. I also added something I had earlier forgotten and two settings needed rework as they were no longer working (we're playing cat and mouse with team yoast). Also tested it with the upcoming WP release, so all is good (I hope) ;)
Many thanks again for all the work you have done to improve the plugin!
Wish you a great weekend!
Piet

@senlin senlin referenced this pull request Aug 18, 2016

Closed

Network activation #18

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.