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.3] Convert email cloack plugin to services #38466

Merged
merged 16 commits into from
Dec 12, 2022

Conversation

laoneo
Copy link
Member

@laoneo laoneo commented Aug 15, 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.

@brianteeman
Copy link
Contributor

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

@laoneo
Copy link
Member Author

laoneo commented Aug 15, 2022

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

@brianteeman
Copy link
Contributor

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

@brianteeman
Copy link
Contributor

Or am I wrong?

@laoneo
Copy link
Member Author

laoneo commented Aug 15, 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.

@wilsonge
Copy link
Contributor

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.

@brianteeman
Copy link
Contributor

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

@laoneo
Copy link
Member Author

laoneo commented Aug 15, 2022

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

@brianteeman
Copy link
Contributor

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

@laoneo
Copy link
Member Author

laoneo commented Aug 15, 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.

@brianteeman
Copy link
Contributor

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

@HLeithner
Copy link
Member

here is the rfc for this joomla/rfc#29

@HLeithner
Copy link
Member

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.

@brianteeman
Copy link
Contributor

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

@brianteeman
Copy link
Contributor

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

@nikosdion
Copy link
Contributor

@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.

@brianteeman
Copy link
Contributor

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

@nikosdion
Copy link
Contributor

@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.

@nikosdion
Copy link
Contributor

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.

@brianteeman
Copy link
Contributor

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.

@nikosdion
Copy link
Contributor

@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.

@brianteeman
Copy link
Contributor

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

@nikosdion
Copy link
Contributor

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

@heelc29
Copy link
Contributor

heelc29 commented Nov 6, 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.

@obuisard
Copy link
Contributor

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.

@obuisard obuisard added this to the Joomla! 4.3.0 milestone Dec 12, 2022
@obuisard obuisard merged commit 7970c2a into joomla:4.3-dev Dec 12, 2022
@obuisard
Copy link
Contributor

Thank you again Allon @laoneo for this series of PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants