[#38976] - Notice: Indirect modification of overloaded property due to emailcloak.php
- Closed
- 18 Oct 2022
- Medium
- Build: 4.2-dev
- # 38976
I discovered some notices in 3rd party extension which occurs due to emailcloak.php.
Below is a quote from a component developer @nikosdion:
The core plugin needs a fix. It assumes there is a property called text in all content objects, regardless of which extension they belong to, and it tries to cloak any emails in them. Since this is triggered
onContentPreparedit executes literally everywhere. It doesn't matter if you have an article, Joomla contact object, a banner, a product in an e-commerce extension or a ticket in a helpdesk extension. It will try to do something with the very likely non-existent property text.Since the ticket object is not a dumb
stdClassobject but a concrete object which implements amagic __getmethod, PHP complains that trying to pass it by reference (see line 86 of the plugin) will not have any effect which is a very correct warning.
https://github.com/joomla/joomla-cms/blob/4.2-dev/plugins/content/emailcloak/emailcloak.phpThe simple solution is that the Joomla email cloaking plugin should actually check there is something to do before trying to do it. Line 53 should read:
if (is_object($row) && property_exists($row, 'text')) {
Joomla 4.2.3
joomla-cms-bot
- | Labels |
Added:
No Code Attached Yet
|
||
Testing instructions can also be to change a file in the core CMS for a test. Did that many time, like that you don't have to make an extra component to illustrate the issue.
I'm with you that no error should be thrown, when the text property doesn't exist. On the other side it would be good , at least to log something when the property doesn't exist. Because this is the "default" behavior that the plugins require a text attribute.
If I'm not wrong then the loadmodule plugin needs to be fixed as well.
@laoneo Good points here. I will go through the content plugins later today to see where this is happening and fix it.
When there is no "text" property we can reasonably assume that it is a custom content type which may not have textual content. Otherwise we'd need to force content to implement a specific Interface instead of being a stdClass so we can have a method, let's call it getTextPropertyName, to return the name of the text content node(s) where content plugins apply. This can be made b/c but I think that's a discussion we can have for Joomla 5 or even Joomla 6 as it requires a lot of changes in the core as well.
For now, I think that checking if a concrete text property exists is fine and can be upgraded in the future if we decide we really want to.
Agree here, especially regarding the interface. Guess this is something when we do the change to concrete events.
I updated the PR and provided a reproduction plugin so you can see how easily our content plugins get thrown off.
@nikosdion Does your PR solve this issue so it can be closed?
@richard67 Yep, it does solve the issue once merged. It also solves the same issue in another two core plugins, killing two (three?) birds with one stone
@nikosdion We usually close issues not when the PR which solves them is merged, we close them already when the PR has been created. That's why I asked. So I can close the issue now.
| Status | New | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-10-18 06:43:29 |
| Closed_By | ⇒ | richard67 |
I opened a draft PR because I'd need to create a demo module or component to give you reproduction.
Do we really need reproduction instructions for such an obvious mistake? @laoneo I'd like your input before I spend time towards that.