friendica/friendica-addons
8
3
Fork 14
Code Issues 3 Pull requests 1 Releases 16 Wiki Activity

Updated URL Replace addon #1483

Merged
MrPetovan merged 18 commits from toddy/friendica-addons:2024.03-rc into 2024.03-rc 2024-03-19 18:00:46 +01:00
Conversation 4 Commits 18 Files changed 6 +137 -54
toddy commented 2024-03-13 23:08:36 +01:00
Contributor
Copy link

Hi,

the new patch from @loma-one is great, I think. I've now updated the URL replace addon to support Instagram links as well. Moreover, the comment from @heluecht makes sense to me as well. #1479 (comment)

So I've added checkboxes for all three services (X, YouTube, Instagram). Now a site admin can select which replacements they want.

This should solve the conflict if the invidious addon is enabled simultaneously.

Hi, the new patch from @loma-one is great, I think. I've now updated the URL replace addon to support Instagram links as well. Moreover, the comment from @heluecht makes sense to me as well. https://git.friendi.ca/friendica/friendica-addons/pulls/1479#issuecomment-185259 So I've added checkboxes for all three services (X, YouTube, Instagram). Now a site admin can select which replacements they want. This should solve the conflict if the invidious addon is enabled simultaneously.
👍 1
toddy added 8 commits 2024-03-13 23:08:37 +01:00 
Based on an idea and patch from loma-one. Thanks!
heluecht commented 2024-03-14 06:44:07 +01:00
Owner
Copy link

Thanks for your work. Only one thing: AFAIK you cannot simply replace the youtu.be domain, since it has got a different URL pattern. Can you check that?

Also: When this addon is a one stop shop solution for all URL, then I suggest to set all the other ones to "deprecated", since it otherwise confuses the admins and users.

Thanks for your work. Only one thing: AFAIK you cannot simply replace the `youtu.be` domain, since it has got a different URL pattern. Can you check that? Also: When this addon is a one stop shop solution for all URL, then I suggest to set all the other ones to "deprecated", since it otherwise confuses the admins and users.
heluecht reviewed 2024-03-14 06:46:53 +01:00
url_replace/url_replace.php Outdated
@ -93,2 +131,4 @@
]);
}
$invidious_server = DI::config()->get('url_replace', 'invidious_server');
heluecht commented 2024-03-14 06:46:53 +01:00
Owner
Copy link

Could you use the third parameter of the get function for the default value? Also it would be nice to use constants for the default URL. See for example the inviduous addon for that.

Could you use the third parameter of the `get` function for the default value? Also it would be nice to use constants for the default URL. See for example the inviduous addon for that.
toddy commented 2024-03-14 08:49:23 +01:00
Author
Contributor
Copy link

Thanks for your review. Unfotunately, the third parameter of get() does not work in this case, I've tried that. When the addon is freshly installed, there is no configuration in the database. In that case, DI::config()->get() gets a NULL value from the database, so the third parameter will be returned as the default value.

However, if the site admin sets another server, then the URL of that server will be stored in the database. If afterwards the site admin deletes that custom server again and leaves the input field blank, then DI::config()->get() will get a result from the database -- an empty string. In that case, the empty string will be returned. This is the reason why I'm checking the value with the empty() function.

An alternative approach would be to specifically check in url_replace_addon_admin_post() if the posted string is empty. If it is, I could then use DI::config->delete() instead of DI::config->set(), that should solve the problem.

Thanks for your review. Unfotunately, the third parameter of get() does not work in this case, I've tried that. When the addon is freshly installed, there is no configuration in the database. In that case, `DI::config()->get()` gets a NULL value from the database, so the third parameter will be returned as the default value. However, if the site admin sets another server, then the URL of that server will be stored in the database. If afterwards the site admin deletes that custom server again and leaves the input field blank, then `DI::config()->get()` will get a result from the database -- an empty string. In that case, the empty string will be returned. This is the reason why I'm checking the value with the `empty()` function. An alternative approach would be to specifically check in `url_replace_addon_admin_post()` if the posted string is empty. If it is, I could then use `DI::config->delete()` instead of `DI::config->set()`, that should solve the problem.
toddy commented 2024-03-14 09:42:06 +01:00
Author
Contributor
Copy link

The alternative approach could look like this:

	$server = rtrim(trim($_POST['nitter_server']), '/');
	if (empty($server)) {
		DI::config()->delete('url_replace', 'nitter_server');
	} else {
		DI::config()->set('url_replace', 'nitter_server', $server);
	}

Should I rewrite the addon in that style?

The alternative approach could look like this: ```php $server = rtrim(trim($_POST['nitter_server']), '/'); if (empty($server)) { DI::config()->delete('url_replace', 'nitter_server'); } else { DI::config()->set('url_replace', 'nitter_server', $server); } ``` Should I rewrite the addon in that style?
heluecht commented 2024-03-15 04:44:18 +01:00
Owner
Copy link

Looks better.

Looks better.
MrPetovan commented 2024-03-15 11:11:42 +01:00
Owner
Copy link

As a general rule, the default value parameter of both Config->get and IConfig->get is now deprecated in favor of using the short ternary operators ?: and ??. This allows for a better control for default values.

As a general rule, the default value parameter of both `Config->get` and `IConfig->get` is now deprecated in favor of using the short ternary operators `?:` and `??`. This allows for a better control for default values.
toddy commented 2024-03-16 11:43:10 +01:00
Author
Contributor
Copy link

I've just pushed a commit which uses this pattern. Please take a look.

I've just pushed a commit which uses this pattern. Please take a look.
👍 1
toddy marked this conversation as resolved
tobias commented 2024-03-14 07:31:46 +01:00
Owner
Copy link

Thanks for your work. Only one thing: AFAIK you cannot simply replace the youtu.be domain, since it has got a different URL pattern. Can you check that?

IIRC you can just replace youtu.be URLs as well.

> Thanks for your work. Only one thing: AFAIK you cannot simply replace the `youtu.be` domain, since it has got a different URL pattern. Can you check that? IIRC you can just replace ``youtu.be`` URLs as well.
toddy commented 2024-03-14 08:50:59 +01:00
Author
Contributor
Copy link

@tobias is right, Invidious is smart enough to understand the same URL structure which YouTube uses, so you only need to replace the server part.

@tobias is right, Invidious is smart enough to understand the same URL structure which YouTube uses, so you only need to replace the server part.
toddy added 1 commit 2024-03-14 09:03:57 +01:00
toddy commented 2024-03-14 09:04:42 +01:00
Author
Contributor
Copy link

@heluecht I've now switched to using constants.

@heluecht I've now switched to using constants.
toddy added 1 commit 2024-03-14 09:05:41 +01:00
toddy added 1 commit 2024-03-14 09:32:04 +01:00
heluecht reviewed 2024-03-15 04:47:04 +01:00
url_replace/url_replace.php Outdated
@ -12,2 +12,4 @@
use Friendica\DI;
const URL_REPLACE_NITTER_DEFAULT = 'https://nitter.net';
const URL_REPLACE_INVIDIOUS_DEFAULT = 'https://yewtu.be';
heluecht commented 2024-03-15 04:47:04 +01:00
Owner
Copy link

Out of interest: Why aren't we using https://invidio.us here? Wouldn't it be better to use the main instances as default values?

Out of interest: Why aren't we using https://invidio.us here? Wouldn't it be better to use the main instances as default values?
tobias commented 2024-03-15 06:55:44 +01:00
Owner
Copy link

You cannot use it to watch something directly, they provide a hub to instances you can use to watch.

You cannot use it to watch something directly, they provide a hub to instances you can use to watch.
heluecht commented 2024-03-16 08:38:38 +01:00
Owner
Copy link

You will see this page where you have to select a server. But when you follow one of these links, your content will be loaded. So you will also be able to chose your individual server each time. Also: I just tested with one video. I had to try several servers, until I found one, that finally was able to display the video. So using https://invidio.us helps here.

You will see this page where you have to select a server. But when you follow one of these links, your content will be loaded. So you will also be able to chose your individual server each time. Also: I just tested with one video. I had to try several servers, until I found one, that finally was able to display the video. So using https://invidio.us helps here.
MrPetovan commented 2024-03-16 09:05:12 +01:00
Owner
Copy link

I agree with @heluecht , let’s use the official portal by default and let admins pick a specific instance later.

I agree with @heluecht , let’s use the official portal by default and let admins pick a specific instance later.
toddy commented 2024-03-16 11:42:33 +01:00
Author
Contributor
Copy link

TBH, I'm not a big fan of that change. After all, it requires two clicks of the user instead of just one to watch the video. All other alternative frontends provided by this addon show the content directly after following the link.

Moreover, yewtu.be has been around for a long time already and seems to be rather reliable. So IMHO it makes for a good and sensible default server. And it has by far the coolest name. :-)

TBH, I'm not a big fan of that change. After all, it requires two clicks of the user instead of just one to watch the video. All other alternative frontends provided by this addon show the content directly after following the link. Moreover, yewtu.be has been around for a long time already and seems to be rather reliable. So IMHO it makes for a good and sensible default server. And it has by far the coolest name. :-)
heluecht commented 2024-03-16 12:02:09 +01:00
Owner
Copy link

Well, the first time I tried the service at yewtub.de, I received an error. I then had to try two other servers to finally find one, that was able to display it to me. So having the choice is a nice thing. Also this is only a default value, so people can enter their own preferred server afterwards - based on their own experience and their regional location.

Well, the first time I tried the service at `yewtub.de`, I received an error. I then had to try two other servers to finally find one, that was able to display it to me. So having the choice is a nice thing. Also this is only a default value, so people can enter their own preferred server afterwards - based on their own experience and their regional location.
toddy commented 2024-03-18 11:12:24 +01:00
Author
Contributor
Copy link

If this is the last discussion point before being able to merge this PR, I'll give in. :-)

If this is the last discussion point before being able to merge this PR, I'll give in. :-)
toddy marked this conversation as resolved
toddy added 1 commit 2024-03-16 11:37:01 +01:00
MrPetovan requested changes 2024-03-16 18:26:18 +01:00
Dismissed
url_replace/url_replace.php Outdated
@ -59,2 +90,3 @@
DI::l10n()->t('Specify the URL with protocol. The default is %s.', URL_REPLACE_NITTER_DEFAULT),
null,
'placeholder="https://nitter.net"',
'placeholder="'.URL_REPLACE_NITTER_DEFAULT.'"',
MrPetovan commented 2024-03-16 18:26:00 +01:00
Owner
Copy link

Code standards: Please wrap operators with spaces.

Code standards: Please wrap operators with spaces.
toddy commented 2024-03-16 20:23:31 +01:00
Author
Contributor
Copy link

Strange, I did run bin/dev/php-cs-fixer/vendor/bin/php-cs-fixer for the file. However, I've seen that you've done some reformatting of my code as well in commit eeb783d71d -- I'm fine with that, but just out of curiosity: Could you please point me to the canonical configuration settings for PHP-CS-Fixer? I thought it's .php_cs.dist in the root directory, but when using that, my code does not get reformatted with spaces between operators.

Strange, I did run `bin/dev/php-cs-fixer/vendor/bin/php-cs-fixer` for the file. However, I've seen that you've done some reformatting of my code as well in commit https://git.friendi.ca/friendica/friendica-addons/commit/eeb783d71d713df23038102048284c6b170634cb -- I'm fine with that, but just out of curiosity: Could you please point me to the canonical configuration settings for PHP-CS-Fixer? I thought it's `.php_cs.dist` in the root directory, but when using that, my code does not get reformatted with spaces between operators.
MrPetovan commented 2024-03-19 17:54:08 +01:00
Owner
Copy link

Sorry about that, we're following the PSR-12 coding standards: https://www.php-fig.org/psr/psr-12/#62-binary-operators

I don't use PHP CS Fixer myself so I'd be hard-pressed to verify the configuration is correct.

Sorry about that, we're following the PSR-12 coding standards: https://www.php-fig.org/psr/psr-12/#62-binary-operators I don't use PHP CS Fixer myself so I'd be hard-pressed to verify the configuration is correct.
toddy marked this conversation as resolved
toddy added 1 commit 2024-03-16 20:26:51 +01:00
heluecht reviewed 2024-03-17 09:53:36 +01:00
url_replace/url_replace.php Outdated
@ -125,3 +184,1 @@
if ($replaced) {
$b['html'] .= '<hr><p><small>' . DI::l10n()->t('(URL replace addon enabled for X, YouTube and some news sites.)') . '</small></p>';
if (count($replacements) > 0) {
heluecht commented 2024-03-17 09:53:36 +01:00
Owner
Copy link

I suggest to compare the original value with the new value to decide, if content had been replaced.

I suggest to compare the original value with the new value to decide, if content had been replaced.
toddy commented 2024-03-17 16:44:46 +01:00
Author
Contributor
Copy link

You're right, that makes more sense. I'll change that probably tomorrow, I'm short on time right now.

You're right, that makes more sense. I'll change that probably tomorrow, I'm short on time right now.
toddy marked this conversation as resolved
toddy added 1 commit 2024-03-18 11:05:52 +01:00
toddy added 1 commit 2024-03-18 11:11:43 +01:00
toddy requested review from MrPetovan 2024-03-18 11:13:05 +01:00
MrPetovan added 3 commits 2024-03-19 17:47:42 +01:00 
- Add default values for config values in url_replace_addon_admin()
- Capitalize brand names
- Please use the URL replace addon instead
- Please use the URL replace addon instead
MrPetovan force-pushed 2024.03-rc from 66c0811ace to 5478552040 2024-03-19 17:56:19 +01:00 Compare
MrPetovan force-pushed 2024.03-rc from 5478552040 to 872a438dcf 2024-03-19 17:57:43 +01:00 Compare
MrPetovan approved these changes 2024-03-19 18:00:41 +01:00
MrPetovan merged commit 11cc359434 into 2024.03-rc 2024-03-19 18:00:46 +01:00
MrPetovan added the
2024.03
 label 2024-03-19 18:01:13 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
MrPetovan
Labels
Clear labels   
2018.09

   
2019.01

   
2019.03

   
2019.06

   
2019.09

   
2019.12

   
2020.03

   
2020.06

   
2020.09

   
2020.12

   
2021.03

   
2021.07

   
2021.09

   
2022.02

   
2022.06

   
2022.09

   
2022.12

   
2023.04

   
2023.05

   
2023.09

   
2024.03

   
2024.06

   
2024.09

   
2024.12

   
dependencies

Pull requests that update a dependency file

  
Hackathon 2021
No labels
2018.09
2019.01
2019.03
2019.06
2019.09
2019.12
2020.03
2020.06
2020.09
2020.12
2021.03
2021.07
2021.09
2022.02
2022.06
2022.09
2022.12
2023.04
2023.05
2023.09
2024.03
2024.06
2024.09
2024.12
dependencies
Hackathon 2021
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: friendica/friendica-addons#1483
No description provided.
Delete branch "toddy/friendica-addons:2024.03-rc"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?