Active check doesn't need duration

[liedekef]

When checking the option using is_admin_notice_active, the length is optional (it is pointless in there anyway ...)

[afragen]

We could just as easily replace

$length      = array_pop( $array );

with

array_pop( $array );

[w3guy]

That's true. Retaining $length is kinda like a comment telling what that array_pop is doing.

[afragen]

Just because $length isn't used does mean it doesn't serve for documentation of sorts. 😉

[afragen]

Will do.

[liedekef]

@collizo4sky : I see the PR has been closed, but the change not merged? Even while using transients, this change is still valid. Oversight?

[afragen]

@liedekef I'm not sure I understand the PR.

Why check strpos($arg,'-') it has to be true as the $length must be appended after a -. A simpler change would be as I described above but it really isn't needed.

[liedekef]

I think you didn't read the whole PR, I changed more lines than just that 1 :-)

[afragen]

I see, but the spec calls for adding the dismissal duration to the data-dismissible=$arg element by appending it with a hyphen. Perhaps we need to make this more explicitly clear in the README?

Logically if (strpos($arg,'-')) { should always resolve true and

} else {
    $option_name = $arg;
}

would never get called.

Can you give an example where it is called?

[w3guy]

@afragen i think we should remove the addition of the duration when calling is_admin_notice_active and hence update the readme too.

To maintain backward compatibility, your above conditional statement will handle this.

[afragen]

@afragen i think we should remove the addition of the duration when calling is_admin_notice_active and hence update the readme too.

To maintain backward compatibility, your above conditional statement will handle this.

I'm not sure I understand. Doesn't it make sense just to use the entire value in the is_admin_notice_active() call? We're asking people use a specific value for the data-dismissable element and then use something different to check against in the function?

Doesn't make logical sense.

[w3guy]

Yor reasoning is why i innitally made it so. We should now leave the readme spec alone.

Permitting the omission of duration when calling is_admin_notice_active wouldn't be that bad a thing 😄

Better still, let's leave it as is and close this PR.

[afragen]

BTW, in our example with data-dismissable="notice-one-forever" the PR will return notice if $arg = 'notice-one'. This means you can't use hyphens anywhere in your unique identifier.

The current code works in all circumstances when the dismiss duration is passed.

[w3guy]

Let's bury this PR in peace 💃

[liedekef]

Concerning the use of hyphens: you're correct, it would result in unwanted behaviour. Now in fact I kept Yorkshire code as a conditional statement to keep a bit of backwards compatibility but of course it could be removed entirely :-) Now on the other hand, your current code works for any added duration, which is also confusing: e.g. is_admin_notice_active("my-note-forever") and is_admin_notice_active("my-note-one") both check for "my-note" ... so I would just remove the duration there and keep it simple.

[liedekef]

(Mobile autocorrect, "Yorkshire" should be "your")

[afragen]

Now on the other hand, your current code works for any added duration, which is also confusing: e.g. is_admin_notice_active("my-note-forever") and is_admin_notice_active("my-note-one") both check for "my-note" ... so I would just remove the duration there and keep it simple.

This is wrong. The only string passed that is a duration is forever otherwise the duration must be a number. Please review the README. If it is unclear, help us make it clearer.

[afragen]

https://github.com/collizo4sky/persist-admin-notices-dismissal/commit/7081d9b0f99e10a7538d88b8006b4c564049522b should ensure the dismissible time is an integer. If a string, other than forever, is passed it will be converted to 0.

https://github.com/collizo4sky/persist-admin-notices-dismissal/commit/0e94f5eb89ac12a48875cd9671bbd24f7f00e65d will ensure default $transient is 1. @collizo4sky you might want to tag another version.

[liedekef]

I'm sorry, my example was maybe poorly chosen. I mean that is_admin_notice_active("my-note-forever"), is_admin_notice_active("my-note-1"), is_admin_notice_active("my-note-5"), etc ... all result in "my-note" being checked, so the duration is irrelevant.

[afragen]

I'm sorry, my example was maybe poorly chosen. I mean that is_admin_notice_active("my-note-forever"), is_admin_notice_active("my-note-1"), is_admin_notice_active("my-note-5"), etc ... all result in "my-note" being checked, so the duration is irrelevant.

Yes, this is true. But it's much easier to tell users to check using the same value they have in data-dismissible in is_admin_notice_active(). It's also less of a potential support issue. The microseconds it will take for the code to strip off the duration is also irrelevant. 😉