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

Add required_value attribute to s2MOP shortcode. #51

Merged
merged 3 commits into from
Sep 15, 2014
Merged

Conversation

raamdev
Copy link
Contributor

@raamdev raamdev commented Sep 13, 2014

Resolves wpsharks/s2member#305.

@jaswsinc I've tested this and it seems to work as expected but would appreciate a code review:

  • If required_type="" contains multiple values (e.g., required_type="ccap|level"), the required_value="" attribute is ignored.
  • The new required_value="" attribute supports multiple values (e.g., required_value="0|1|2" or required_value="ccap1|ccap2|ccap3")

Tested with these shortcodes:

[s2MOP required_type="ccap" required_value="free_gift"]This content required a Custom Capability called <code>free_gift</code>[/s2MOP]
[s2MOP required_type="level" required_value="0|1"]You were trying to access a Post that requires Level <code>0</code> or <code>1</code>[/s2MOP]

if(empty($_g['_s2member_req']['type']) || !in_array($_g['_s2member_req']['type'], $attr['required_type'], TRUE))
return '';

if(isset($attr['required_value']) && ( count($attr['required_type']) !== 1 || !in_array($_g['_s2member_req'][$_g['_s2member_req']['type']], $attr['required_value'], TRUE)))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaswsinc I'm not sure if there is a cleaner way referencing the value of one array ($_g['_s2member_req']['type']) inside another, as I did here:

$_g['_s2member_req'][$_g['_s2member_req']['type']]

It looks hard to read, but the only alternative--assigning that value to a separate variable and then using the variable in the array--seemed like a lot of extra code (assigning the value and then unsetting the value).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be fine to create another variable to help make this clearer. What you have is fine too. I just mean to say that (for me) I would be OK with another few lines of code if that makes it's easier to read, understand, and come back to in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be fine to create another variable to help make this clearer.

Cool. Thanks! I've updated this line for clarity.

@jaswrks
Copy link
Contributor

jaswrks commented Sep 14, 2014

The new required_value="" attribute supports multiple values (e.g., required_value="0|1|2" or required_value="ccap1|ccap2|ccap3")

Cool! Love the way you set things up.

I was looking at this line. I think we should add required_value to that array of shortcode attributes that are accepted (see also: shortcode_atts reference).

Also, in this block the use of isset() seems a little misleading. These values will always be set here, but they might be empty in some cases.

- Add required_value attribute to shortcode_attrs()
- Remove unnecessary isset() conditionals
- Clarify required_value attribute conditional
@raamdev
Copy link
Contributor Author

raamdev commented Sep 15, 2014

I was looking at this line. I think we should add required_value to that array of shortcode attributes that are accepted (see also: shortcode_atts reference).

Weird. I thought I added that there. Thanks for catching that.

Also, in this block the use of isset() seems a little misleading. These values will always be set here, but they might be empty in some cases.

Ah, good point. I removed the isset() checks in that block of code. You're right: they serve no purpose. In fact, we don't even need to verify if the values are empty or not, since the following array_intersect() statement validates the shortcode attribute values anyway.

jaswrks pushed a commit that referenced this pull request Sep 15, 2014
Add required_value attribute to s2MOP shortcode.
@jaswrks jaswrks merged commit 8423f9f into 000000-dev Sep 15, 2014
@jaswrks
Copy link
Contributor

jaswrks commented Sep 15, 2014

Great, thanks @raamdev 👍

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.

2 participants