Fixed non-object notice on the post edit screen

[robneu]

I tried to do this with the least amount of code changes to avoid potential conflicts. There are probably cleaner ways to do this, and it might also but sufficient to make sure that var isn't null, but this seemed like the quickest/least invasive way to handle it.

This should close #55

[robneu]

It might also be a good idea to spin up an instance of the CPT class regardless of the settings since it's supposed to be a property of the main plugin class. Because the class doesn't really execute any code until it's run, I don't think there's much reason not to instantiate it.

If you agree, I'll update the PR to instantiate the class before the setting check.

[NateWr]

Thanks @robneu!

I'm trying to think through always instantiating the CPT class. One thing I'm thinking is that maybe it's not a good idea since we use the common location post type slug. For instance, if we were to truncate the conditional check in the relevant code in this PR:

if ($this->cpts->location_cpt_slug === $post->post_type ) {
    wp_enqueue_style( 'bpfwp-admin-location', BPFWP_PLUGIN_URL . '/assets/css/admin.css' );
}

This would load the CSS file on the post editing screen of any post type named location.

That said, I'm not against always instantiating the custom post type class. But if we do that, I think we should add a helper is_location_active method on that class or the global object which wraps up the check against the settings. We'll then need to check for other places in the code base where it ought to be used instead of a direct check against ->cpts.

(I'm also happy to merge this as-is and save further adjustments for later.)

[robneu]

Personally, I think you might be worrying a little too much. If someone has multiple plugins registering a CPT with the same slug, there's going to be quite a few issues unrelated to the styles you're loading. I don't know that trying to avoid issues there is worth any extra effort.

FWIW, I was a little bit against some of the CPT standardization stuff because of this kind of thing. The meta standards seem like a good idea, but standardizing the slugs seems like a potential problem. Besides, porting over one CPT to another is pretty trivial using something like Post Type Switcher.

Anyways, not to get too far off track... I think spinning up an instance of that class would be fine in most cases and the ones where it isn't would have issues outside of your control anyway.

[NateWr]

I see your point. But with this plugin many (maybe most) users will be running it without the locations feature enabled. So the post type will never be registered.

I'm particularly thinking of all those mega themes with custom post types baked in. Someone may not even be using a location post type but they could have one registered on the system. It's certainly an outlier. But if I can guard against it I'd like to.

So, again, I'm not opposed to spinning up that class on load, but maybe let's check the multi-location setting before enqueueing the admin stylesheet:

if ( $this->settings->get_setting( 'multiple-locations' ) && $this->cpts->location_cpt_slug === $post->post_type ) {
  // yadayadayada
}

[robneu]

That sounds like a reasonable middle ground to me. I can update the PR to that effect, or if you'd prefer you can just close this out and make the change directly.

[NateWr]

Go ahead and update the PR if you've got a moment. Otherwise no worries I can get it taken care of soon.

[robneu]

Should be all set. Tested it out locally and everything seems 👌 on my end. Let me know if anything needs adjusted!

[NateWr]

Merged, cheers! Are you waiting on this fix urgently anywhere? Or are you ok waiting for this to go out with the next meaningful update?

[NateWr]

ps - you ok with this? https://github.com/NateWr/business-profile/commit/1223a135aaefe35ef3d1ecbc6a2b02a70d918bbf

[robneu]

Yeah, of course! Thanks for the credit. 😀

No rush on the fix, I patched it on my local installs just because I keep debugging turned on.