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

Core-ify "Random Keywords" plugin #2367

Merged
merged 15 commits into from Oct 2, 2019

Conversation

@dgw
Copy link
Member

commented Feb 27, 2018

This is still WIP, don't merge yet. In particular, the check desired in #1877 for whether the standalone plugin is active doesn't do anything/might even crash the whole plugin (I've not tested it yet).

Mostly I'm looking for feedback on whether or not it's worth keeping the commit history of the original plugin. I did this with a merge using --allow-unrelated-histories. For the final PR, I can either prefix the merged-in commits with something so it's clear they pertain to the plugin, not YOURLS itself, or just remove the history and do a copy-paste import instead. What say you @ozh @LeoColomb?

Whatever the way forward, it's clear that having this functionality as a core plugin will (hopefully) reduce the number of issues/comments asking how to avoid sequential shorturls.

ozh and others added 7 commits Jan 27, 2014
Using str_shuffle means that each character can only occur once in the generated string. This reduces the number of permutations possible. For example, using base 36, and 4 characters, there are 36^4 = 1.68M, but the current function can only generate 36*35*34*33 = 1.41M of these. This alternative will use the whole range of possible strings.
Allow character to appear more than once
* Rename plugin and its functions
* Update README with new name
* Update license with current year
* Make plugin refuse to activate if the old 'Normal plugin' version
  is still active.
This was formerly an external plugin named `random-keywords`.

History from the `random-keywords` repo is kept.
@dgw dgw added this to the 1.8 milestone Feb 27, 2018
@dgw dgw self-assigned this Feb 27, 2018
@dgw dgw added the not ready label Feb 27, 2018
@ozh

This comment has been minimized.

Copy link
Member

commented Feb 27, 2018

I for one really don't care about the git log and commit message history :) (I mean, nicely put commit messages are nice to have, but they shouldn't dictate the way one works)

@LeoColomb LeoColomb added this to Ready in YOURLS Mar 14, 2019
@LeoColomb LeoColomb removed the wip label Mar 24, 2019
@dgw

This comment has been minimized.

Copy link
Member Author

commented Sep 21, 2019

@LeoColomb I don't exactly remember what I still wanted to do with this. There was something, but I don't think I have time to look from a computer until tomorrow.

@LeoColomb

This comment has been minimized.

Copy link
Member

commented Sep 21, 2019

@dgw #1877 (comment)

(this would require a renaming of the functions and a check for previous version activated)

😉

@dgw

This comment has been minimized.

Copy link
Member Author

commented Sep 21, 2019

Oh yeah. Maybe also a real option instead of $ozh_random_shorturl['length'] = 5; that needs to be edited in the file, since upgrading YOURLS would overwrite changes to that.

Virinum and others added 2 commits Jan 3, 2019
If strlen($possible) is for example 36, rand could also return 36.
But substr starts with position 0. So we have to subtract 1 from
strlen($possible).

----

Yoinked from YOURLS/random-keywords#10 by @dgw
@dgw

This comment has been minimized.

Copy link
Member Author

commented Sep 22, 2019

So I had a bit of time tonight after all. There's already a "conflict preventer" function that checks if the old plugin is active. It might not actually prevent loading this one in that case; I haven't had a chance to test it. (Honestly, I've been unable to find much useful documentation about how hooks are supposed to work. Do I just return false?)

But I noticed there was a merged PR in the other repo and pulled in those changes (tiny one-liner), and implemented a settings page which should work. 🤞

@LeoColomb LeoColomb requested a review from ozh Sep 23, 2019
@ozh

This comment has been minimized.

Copy link
Member

commented Sep 24, 2019

Yep, saw this PR, will have time to look at it tomorrow :)

@ozh

This comment has been minimized.

Copy link
Member

commented Sep 25, 2019

Nope, won't work ...

There's a missing ( on line 40 : if yourls_is_active_plugin( 'random-keywords' ) should be if (yourls_is_active_plugin( 'random-keywords' )) 😄

Plus, I think checking with yourls_is_active_plugin() isn't foolproof enough: if someone has installed the plugin in directory 'random-keyword(no trailing s) orrandom-url-like-bitly`, will this work? I'm not sure how this works but I think it's based on the directory name.
I think a more foolproof way would be to check if function ozh_random_keyword() already exists

Also, it's hooked onto 'activated_random-shorturls' so I guess it will echo the warning after activation anyway, and you'll end up with both plugins enabled. I don't think we have something early enough before plugin activation

Maybe a simple way would be something like:

if (function_exists('ozh_random_keyword')) {
   yourls_add_notice('deactivate your plugin, it's obsolete') or something like this
} else {
    all the yourls_add_action and filter to actually register the newer plugin
} 

Thoughts ?

@dgw

This comment has been minimized.

Copy link
Member Author

commented Sep 27, 2019

I was going to actually do some of this now, but I've run out of time. 😔 So it'll get done during the next block of free time I (think I) have.

Also, it's hooked onto 'activated_random-shorturls' so I guess it will echo the warning after activation anyway, and you'll end up with both plugins enabled.

The idea was to block activating the plugin entirely, but it looks like there's no hook capable of doing that. All of the plugin activation hooks run after the activation itself is already finalized. So, I'll have to overwrite that (stub) implementation with a different approach—probably based on detecting the old plugin's function name, yes.

There's a missing ( on line 40 : if yourls_is_active_plugin( 'random-keywords' ) should be if (yourls_is_active_plugin( 'random-keywords' )) 😄

I'm spending too much time in Python these days… 🤣 But replacing the conflict preventer as discussed above will eliminate this error anyway.

@dgw dgw dismissed LeoColomb’s stale review Sep 27, 2019

Code is nowhere close to ready, so having an approval on it is very misleading!

YOURLS automation moved this from Ready to Reviewing Sep 27, 2019
@dgw dgw moved this from Reviewing to In progress in YOURLS Sep 27, 2019
The conflict preventer function was broken anyway (ineffective logic AND
incorrect syntax)
@dgw

This comment has been minimized.

Copy link
Member Author

commented Sep 29, 2019

There, @ozh, I think that's more like what you were suggesting. Better?

@ozh

This comment has been minimized.

Copy link
Member

commented Oct 2, 2019

Hmm, still doesn't work: having old one activated and activating new one, I get the expected notice but the plugin is still activated :

Random ShortURLs plugin cannot function unless Random Keywords is removed first.
Plugin has been activated

Will look into it

@ozh

This comment has been minimized.

Copy link
Member

commented Oct 2, 2019

Meh. I'm dumb. Works completely as expected. :-)

ozh added 3 commits Oct 2, 2019
@ozh
ozh approved these changes Oct 2, 2019
YOURLS automation moved this from In progress to Ready Oct 2, 2019
@ozh ozh merged commit c5a65b7 into YOURLS:master Oct 2, 2019
2 checks passed
2 checks passed
Scrutinizer 2 new issues, 5 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
YOURLS automation moved this from Ready to Done Oct 2, 2019
@dgw dgw deleted the dgw:core-ify_random_keywords branch Oct 2, 2019
LeoColomb added a commit to YOURLS/awesome-yourls that referenced this pull request Oct 3, 2019
Plugin integrated in the core
Ref YOURLS/YOURLS#2367
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
YOURLS
  
Done
5 participants
You can’t perform that action at this time.