PR-4.3-dev Pending

User tests: Successful: Unsuccessful:

avatar laoneo
laoneo
15 Aug 2022

Summary of Changes

Converts the content email cloack plugin to services.

Testing Instructions

  • Create an article with an email address in the description and featured flag set to yes.
  • Open the article on the front

Actual result BEFORE applying this Pull Request

There is a "joomla-hidden-mail" tag around the mail address.

Expected result AFTER applying this Pull Request

There is a "joomla-hidden-mail" tag around the mail address.

avatar laoneo laoneo - open - 15 Aug 2022
avatar laoneo laoneo - change - 15 Aug 2022
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 15 Aug 2022
Category Front End Plugins
avatar brianteeman
brianteeman - comment - 15 Aug 2022

Maybe this is a stupid question, if so I apologise, but I woiuld rather ask it and potentially look stupid than see a problem and ignore it.

Was it possible for an extension to extend PlgContentEmailcloak ?

If it was then doesn't this PR break that as its now called Emailcloak

cc0cf90 15 Aug 2022 avatar laoneo final
avatar laoneo laoneo - change - 15 Aug 2022
Labels Added: PR-4.3-dev
avatar laoneo
laoneo - comment - 15 Aug 2022

Yes, every class rename will not work anymore for the old class name, except when you do an alias.

avatar brianteeman
brianteeman - comment - 15 Aug 2022

So that makes this and the other pr a backwards compatible break that cannot be done until j5

avatar brianteeman
brianteeman - comment - 15 Aug 2022

Or am I wrong?

avatar laoneo
laoneo - comment - 15 Aug 2022

It's not covered by our BC policy. So from this point of view the changes are fine in 4.x. If you want to be on the safe side, then you can create an alias in the file https://github.com/joomla/joomla-cms/blob/4.2-dev/libraries/extensions.classmap.php and remove the final keyword from the new classes.

avatar wilsonge
wilsonge - comment - 15 Aug 2022

I think from a theoretical standpoint it's very unlikely people would extend plugins as they are pretty single purpose and fairly hardcoded to the extension. I think we might have to take any helper classes associated with a plugin on a plugin by plugin basis however.

avatar brianteeman
brianteeman - comment - 15 Aug 2022

So what is the benefit of changing the name? If its only cosmetic then surely its better to be safe than sorry and not break things

avatar laoneo
laoneo - comment - 15 Aug 2022

As the core acts as example for extension developers, the plugins should not be written the legacy way.

avatar brianteeman
brianteeman - comment - 15 Aug 2022

at the risk of breaking someone's site with no advance notice? thats hardly semantic versioning

avatar laoneo
laoneo - comment - 15 Aug 2022

From semver.org:

you first need to declare a public API

From https://developer.joomla.org/development-strategy.html (BC policy):

All PHP code in the /libraries folder which is not flagged as private is considered to be part of the Joomla API and subject to backwards compatibility constraints.

If you want to extend the Joomla BC policy, then this needs to be brought to Production, so we can expand our policy. As long as we have a clear statement what our public API is, changes outside of it don't go under the BC policy.

I mean, I agree that we should do our best to not introduce any disruptive change, so I proposed you a solution how you can prevent this, if you think something must be changed.

avatar brianteeman
brianteeman - comment - 15 Aug 2022

This is a disruptive change that is not required at this time

avatar HLeithner
HLeithner - comment - 15 Aug 2022

here is the rfc for this joomla/rfc#29

avatar HLeithner
HLeithner - comment - 15 Aug 2022

btw the name change would be the smalles problem. The class is now final (which is good) that means it can't be extended anymore.

avatar brianteeman
brianteeman - comment - 15 Aug 2022

That rfc which is still awaiting a vote is very clear that this pr would not satisfy the requirements

avatar brianteeman
brianteeman - comment - 15 Aug 2022

btw the name change would be the smalles problem. The class is now final (which is good) that means it can't be extended anymore.

it was not final at the time I first commented - just as it is not final in #38446

avatar nikosdion
nikosdion - comment - 15 Aug 2022

@brianteeman We had already discussed this point. Core plugins are NOT covered by the b/c promise. You are not supposed to extend from core plugins. We have already done that with the Page Cache plugin. This will have to happen with all core plugins so they are written the Joomla 4 way — except WebAuthn and plugins added after the 4.1 release i.e. after we had that discussion.

Beyond the fact that you should not extend core plugins, I have not seen any plugin in the wild extending from a core plugin anyway. So let's weigh these two problems. On one hand we have the HYPOTHETICAL problem that a 3PD plugin might extend a core plugin even though a. we have no evidence that this might have ever happened and b. you were never supposed to extend from core plugins — or any plugin, really! — anyway. On the other hand we have the REAL problem that 3PDs looking for how the heck to write a Joomla plugin look at the core code and see than ~95% of the core plugins are written the legacy way, thereby inferring that this is the recommended way to do things, therefore resulting in plugins which will be inoperable in future Joomla versions. Between the hypothetical and the real issue we all agreed it is far more important to solve the real issue.

It would have been a far different discussion if the PR proposed renaming a core Model. In that case 3PD extensions can and do reference core models. Renaming a Model in the middle of a release cycle would be a b/c break. Therefore Models are only renamed in major releases, like what happened with com_content in Joomla 4. The same applies for core components' helpers. This does not apply for core components' Controllers, Views, Fields etc.

The thing is, when Joomla 3 was written either @internal did not exist in the phpDoc specification or nobody had heard of it (can't remember what) so nobody could add this marker to what are essentially internal classes. Yet, everyone understood that and didn't screw with core plugins if they knew what was good for them.

avatar brianteeman
brianteeman - comment - 15 Aug 2022

We had already discussed this point. Core plugins are NOT covered by the b/c promise. You are not supposed to extend from core plugins.

We had? Where?

On the other hand we have the REAL problem that 3PDs looking for how the heck to write a Joomla plugin look at the core code and see than ~95% of the core plugins are written the legacy way, thereby inferring that this is the recommended way to do things, therefore resulting in plugins which will be inoperable in future Joomla versions.

agreed and why I finaly got it when @nibra talked about it.

At a minimum the changes being made should be documented and preferably before asking people to review the implementation of those changes. Thats how and why mistakes happen such as forgetting the final or not declaring the internal

avatar nikosdion
nikosdion - comment - 15 Aug 2022

@brianteeman #35986 (comment) and the discussion onwards. Please do read all the way to the bottom of that PR discussion.

The discussion happened in November 2021, before Joomla 4.1's release. The results of the discussion were implemented in #35986 by yours truly. The refactored plugin was merged in the Joomla 4.2 development branch.

avatar nikosdion
nikosdion - comment - 15 Aug 2022

I would also like to note that this is the same place where the decision was made / we all agreed that it's ridiculous that Joomla's core plugins do not follow the way Joomla 4 plugins must be written and that it's something that needs to be addressed before 5.0. Discussing the same thing on every single plugin being refactored would be counterproductive.

As for bugs and typos, there are only two kinds of developers. Those who admit they make them and those who are bad liars.

avatar brianteeman
brianteeman - comment - 15 Aug 2022

I guess you have a completely different idea to me on what "document it" and a "decision was made" mean. Which surprises me. I've no idea how anyone is supposed to know about that stuff other than the handful of people in that pull request.

I fully understand the argument and the reasoning behind it. I just feel that this is exactly the sort of thing you usually scream about because there is no documentation and no way you could know about it.

avatar nikosdion
nikosdion - comment - 15 Aug 2022

@brianteeman Every so often I question the logic of a core decision, you link me to an obscure comment of a PR I was not even aware of and tell me it was a public discussion, I could have joined in at the time. Now that you are on the receiving end you are enraged instead of defiant. So, no, I do not agree with that in the slightest. I think this kind of comments is toxic. I had tried to tell you but you wouldn't listen so I figured a taste of your own medicine might help drive the point home ?

For real now, I'd personally prefer a detailed stream of information coming out of production every time a decision is made which even remotely affects 3PDs. Preferably in an RSS feed so we can track it. Heck, even blog posts / magazine articles the way bloggers tracking PHP RFCs do.

I have no badge, though. I cannot make a decision for the project to communicate or even make such a proposal. If you are accusing me for the project not communicating something important you are barking up the wrong tree. You very well know where I stand on the matter.

Also note that communicating every decision which affects 3PDs requires people who can communicate, not just people who can write code and architect software. Would you volunteer for that? Maybe @bembelimen and @softforge could incorporate that in the PR merge process if there were people who could actually take on that task.

avatar brianteeman
brianteeman - comment - 15 Aug 2022

I really dont know why I bother. I asked a simple question

avatar nikosdion
nikosdion - comment - 15 Aug 2022

@brianteeman I am not sure if you are a hypocrite or a narcissist. Either way, I wasted too much time with you already.

avatar heelc29
heelc29 - comment - 6 Nov 2022

I have tested this item successfully on 7ad9ce3


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

avatar heelc29 heelc29 - test_item - 6 Nov 2022 - Tested successfully
avatar joomla-cms-bot joomla-cms-bot - change - 12 Dec 2022
Category Front End Plugins Administration com_fields com_installer Front End com_content Libraries Plugins
avatar joomla-cms-bot joomla-cms-bot - change - 12 Dec 2022
Category Front End Plugins Administration com_fields com_installer com_content Libraries Front End Plugins
avatar obuisard obuisard - test_item - 12 Dec 2022 - Tested successfully
avatar obuisard
obuisard - comment - 12 Dec 2022

I have tested this item successfully on cafd141


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

avatar obuisard obuisard - close - 12 Dec 2022
avatar obuisard obuisard - merge - 12 Dec 2022
avatar obuisard obuisard - change - 12 Dec 2022
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2022-12-12 12:25:59
Closed_By obuisard
avatar obuisard
obuisard - comment - 12 Dec 2022

Thank you again Allon @laoneo for this series of PRs

Add a Comment

Login with GitHub to post a comment