Set UTM source to slug of site name

xwp / unsplash-wp

GNU General Public License v2.0

9 stars 3 forks source link

Set UTM source to slug of site name #85

Closed pierlon closed 4 years ago

pierlon commented 4 years ago

Summary

Fixes UVP-180.

Checklist

[ ] My pull request is addressing an open issue (please create one otherwise).
[ ] My code is tested and passes existing tests.
[ ] My code follows the Contributing Guidelines (updates are often made to the guidelines, check it out periodically).

pierlon commented 4 years ago

We should also make the fetching of the other settings static (access key and secret key).

spacedmonkey commented 4 years ago

We should also make the fetching of the other settings static (access key and secret key).

No we shouldn't static method are really against the point of objects.

Wouldn't it be easier to define this as a constant or a function in the main plugin file?

pierlon commented 4 years ago

No we shouldn't static method are really against the point of objects.

Agreed. The issue here is that there is no reason to instantiate these values on every request. They should instead be moved to a class that houses them and are able to be retrieved statically.

pierlon commented 4 years ago

Wouldn't it be easier to define this as a constant or a function in the main plugin file?

As stated earlier, I think these values should go into their own class. There's no need to overload the plugin class. Thoughts?

spacedmonkey commented 4 years ago

It's own class feels like overkill for something that could be as simple as.

if( defined( 'UNSPLASH_UTM_SOURCE') ) {
     define('UNSPLASH_UTM_SOURCE', sanitize_title_with_dashes( get_bloginfo( 'name' ) ));
}

spacedmonkey commented 4 years ago

As stated earlier, I think these values should go into their own class. There's no need to overload the plugin class. Thoughts?

Creating a class for this is massively overkill. Adding a new class will take time to setup tests etc. Creating an object take resources and has overhead. Setting up a constant with a option that is already in memory seems to have little downside to me.