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

[4.0] Remove com_mailto #25516

Merged
merged 1 commit into from
Feb 24, 2020
Merged

[4.0] Remove com_mailto #25516

merged 1 commit into from
Feb 24, 2020

Conversation

brianteeman
Copy link
Contributor

with #24025 we removed the mail a link to a friend functionality. But I didnt remove the com_mailto component. Afaict this component was only used for the mail to friend functionality so I have now removed all traces of the component (I hope)

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Jul 11, 2019
@C-Lodder
Copy link
Member

What about those migrating from 3.x to 4.0? Doesn't the com_mailto directory need removing too?

@brianteeman
Copy link
Contributor Author

I "think" the pr does that. For the migrating users then it will be handled by script.php at update time

@zero-24
Copy link
Contributor

zero-24 commented Jul 16, 2019

will be handled by script.php at update time

but the required changes to that file are missing in the script.php right now?

I have also added the Documentation required label as this should be included in the docs too :)

@brianteeman
Copy link
Contributor Author

but the required changes to that file are missing in the script.php right now?

I meant that the script.php will be updated near to release time in one go. If you noticed you would see that there have not been any commits to that file during the development of J4 with individual PR

I have also added the Documentation required label as this should be included in the docs too :)

What documentation

@zero-24
Copy link
Contributor

zero-24 commented Jul 16, 2019

@zero-24
Copy link
Contributor

zero-24 commented Jul 16, 2019

Maybe an dedicated page why or better that we removed it makes sense too.

@brianteeman
Copy link
Contributor Author

its not a bc issue

better to spend your time making security headers actually work

@zero-24
Copy link
Contributor

zero-24 commented Jul 16, 2019

better to spend your time making security headers actually work

I'm sorry is there a reason to personally attact me? Can you please tell me what is the problem at the security headers? As there are already bigger changes planed when the current PR gets merged, I can check on the issues too.

@brianteeman
Copy link
Contributor Author

it doesnt work at all.

@zero-24
Copy link
Contributor

zero-24 commented Jul 16, 2019

it doesnt work at all.

hmm please share your configuration and the steps to reproduce with me. I have just tried it on php 7.3 with the latest build and it worked correctly, but I'm happy to debug and improve what ever is needed. Thanks :)

@brianteeman
Copy link
Contributor Author

see #25592 had to guess what to do as I couldnt find any documentation

@uglyeoin
Copy link
Contributor

@brianteeman please can you provide testing instructions and i will test this PR so it can be accepted. I assume I can simply apply this patch then search in "manage extensions" for "mail to" and if it does not exist I can pass this PR?

@uglyeoin
Copy link
Contributor

Do we need to install sample data? I get this error:

The file marked for modification does not exist: installation/sql/mysql/sample_testing.sql

When trying to install the "testing" sample data I also get another error. I know that's unrelated but can you confirm it makes this test impossible. Or is the sample_testing.sql part of this pull request unecessary?

@brianteeman
Copy link
Contributor Author

conflicts resolved.

@uglyeoin that will resolve your patchtester error. As for testing instructions you got it right

@uglyeoin
Copy link
Contributor

God almighty. Who do I have to butter up to get that tooltip off the issue number in Patchtester when I try to type it in? It's destroying my brain.

@uglyeoin
Copy link
Contributor

uglyeoin commented Oct 26, 2019

I cannot apply the patch.

The patch could not be applied because it conflicts with a previously applied patch: installation/sql/mysql/joomla.sql

I have checked and this is the only patch applied, I have reverted all others.

@Quy
Copy link
Contributor

Quy commented Oct 26, 2019

God almighty. Who do I have to butter up to get that tooltip off the issue number in Patchtester when I try to type it in? It's destroying my brain.

Are you referring to the alert? Try this: #25850 (comment)

@brianteeman
Copy link
Contributor Author

@uglyeoin report that to the patchtester repo. I cannot confirm with the old version of patchtester I use.

@uglyeoin
Copy link
Contributor

@Quy that isn't the problem, this is the problem. joomla-extensions/patchtester#243 (comment)

I suggested the solution. I'd do a pull request but I'm really not sure how to do it when it comes to compiled CSS. If someone can point me at instructions I'll give it a go.

@brianteeman
Copy link
Contributor Author

conflicts resolved

@@ -101,23 +100,7 @@ public function create($category, $params, $attribs = array(), $legacy = false)
*/
public function email($article, $params, $attribs = array(), $legacy = false)
{
$uri = Uri::getInstance();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be deprecated in v3.10 and remove in v4.0 like you did in #25502?

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 am not sure - ask @wilsonge

Copy link
Contributor

Choose a reason for hiding this comment

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

You had a similar conversation with @wilsonge that resulted in #27067.

Copy link
Contributor

Choose a reason for hiding this comment

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

Deprecate + remove

Copy link
Contributor

Choose a reason for hiding this comment

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

@brianteeman I assume it is go since @wilsonge commented to deprecate & remove.

@jwaisner
Copy link
Member

jwaisner commented Jan 4, 2020

I have tested this item ✅ successfully on 3f180e9


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25516.

@brianteeman
Copy link
Contributor Author

Is there any point in me resolving conflicts again or am I wasting my time

@brianteeman
Copy link
Contributor Author

conflicts resolved and deprecation pr created

@zero-24
Copy link
Contributor

zero-24 commented Feb 24, 2020

Deprecation PR merged 👍

@HLeithner HLeithner merged commit 671a4fa into joomla:4.0-dev Feb 24, 2020
@HLeithner
Copy link
Member

Thanks for removing this awful feature.

@brianteeman
Copy link
Contributor Author

Finally!!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants