Bug #87064
closed
Deprecated public properties can't be accessed after removal
100%
Description
While it is a good idea to convert public to protected properties to avoid manipulation from the outside, it will make access to these properties within hooks provided within the same class as these properties impossible.
Example taken from the Gridelements new item wizard hook:
Within the NewContentElementController there are
/**
* @var array
*/
protected $deprecatedPublicProperties = [
'id' => 'Using $id of NewContentElementController from the outside is discouraged as this variable is used for internal storage.',
'sys_language' => 'Using $sys_language of NewContentElementController from the outside is discouraged as this variable is used for internal storage.',
'R_URI' => 'Using $R_URI of NewContentElementController from the outside is discouraged as this variable is used for internal storage.',
'colPos' => 'Using $colPos of NewContentElementController from the outside is discouraged as this variable is used for internal storage.',
'uid_pid' => 'Using $uid_pid of NewContentElementController from the outside is discouraged as this variable is used for internal storage.',
'modTSconfig' => 'Using $modTSconfig of NewContentElementController from the outside is discouraged as this variable is used for internal storage.',
'doc' => 'Using $doc of NewContentElementController from the outside is discouraged as this variable is used for internal storage.',
'content' => 'Using $content of NewContentElementController from the outside is discouraged as this variable is used for internal storage.',
'access' => 'Using $access of NewContentElementController from the outside is discouraged as this variable is used for internal storage.',
'config' => 'Using $config of NewContentElementController from the outside is discouraged as this variable is used for internal storage.',
];
Now the whole class is handed over to hook objects via
$hookObject->manipulateWizardItems($wizardItems, $this);
public function manipulateWizardItems(&$wizardItems, $parentObject)
As soon as these properties are really deprecated with CMS 10, it won't be possible anymore to get $parentObject->id or $parentObject->colPos from within the hook, which is essential information while rendering the wizard items.
Solution: Since the PublicPropertyDeprecationTrait is used quite a lot all over the core, we should make sure that properties which are handed over to hooks always
provide a getter to access their values while keeping them protected.
Updated by Coders.Care Extension Team almost 6 years ago
- Description updated (diff)
Updated by Coders.Care Extension Team almost 6 years ago
- Description updated (diff)
Updated by Georg Ringer almost 6 years ago
I wouldn't want getters for every property as those as those could be just internal. I would propose adding a new hook or request getters for specific properties
Updated by Coders.Care Extension Team almost 6 years ago
Good point.
Actually it does not matter how this will be resolved technically as long as necessary information is still made available.
In this specific case it would be enough to provide an array of the necessary key/value pairs either as additional parameter or maybe as a replacement for $parentObject, since the parent object would be more or less inaccessible anyway.
Should be checked what fits the other cases best. In this case i.e. having just a few properties like id, uid_pid, sys_language and colPos would be enough to determine the context for additional wizard items.
Updated by Georg Ringer over 5 years ago
- Status changed from New to Closed
I am closing this issue as it is too general. if you are missing properties in a hook, please create a new issue per hook. thanks!
Updated by Jo Hasenau over 5 years ago
- Status changed from Closed to Accepted
Actually this had been discussed already with Benni Mack and we agreed that these properties must be made available again.
The reason for the properties to become protected was to prevent manipulation from the outside, still they need to be available for requests from the outside unless they are definitely private or internal.
That's why we have to provide getters in general and not just for specific hooks, since in most of the hooks the parent object has been available as a whole for ages.
So please let's try to reach a consensus before closing this issue again.
Updated by Benni Mack over 5 years ago
Hi Joey,
can you please list of the properties you need to access? Do we really need ALL of them?
Thanks for the info
Updated by Jo Hasenau over 5 years ago
The intention of the ticket was not just to make things work again for gridelements, since there are quite some hooks just handing over the whole parent object with fully accessible variables.
Since this is still working that's no big deal with CMS 9, but there is no way to access the necessary information with CMS 10 anymore, not even a workaround. So a lot of extensions making use of those hooks will break without a chance to get back that information.
I can check that especially for the extensions we are maintaining and report it here, but there will be more extensions out there and I would just like to make sure, they won't get lost with CMS 10.
IMHO we should always provide getters for protected variables and mark only those, that really need to be kept secret due to security reasons as private without providing a getter.
Updated by Georg Ringer over 5 years ago
not every property needs a getter as those makes it then again public API which makes it impossible to refactor stuff.
if extensions require a special property and there is a reason it is fine to hand that to hooks
Updated by Benni Mack over 5 years ago
Hey Joey,
the idea is that we can e.g. remove the dependency to DocumentTemplate - this is how we can shape the API properly. We had the same with TemplateService, PageRepository, HMENU to have better maintainability in the future. So we then know what is used in the wild. Going "public" with all properties does not get us any further.
Updated by Jo Hasenau over 5 years ago
So I got the intention wrong, since I just thought they would be protected to avoid changes from outside the class.
If those properties have been "hidden" on purpose, actually each of them should be marked as private and we should make sure extension maintainers will get informed that there will be serious breaking changes with CMS 10.
The question is, how we can reach out to extension maintainers to collect the necessary information. Current comments like
Using $id of NewContentElementController from the outside is discouraged as this variable is used for internal storage
are not that helpful, since they just tell people "don't use it, since it's none of your business", which is wrong regarding the situation with hooks, but they don't encourage communication about the necessary variables. So maybe there is another way to get people involved.
Updated by Benni Mack over 5 years ago
Jo Hasenau wrote:
So I got the intention wrong, since I just thought they would be protected to avoid changes from outside the class.
If those properties have been "hidden" on purpose, actually each of them should be marked as private and we should make sure extension maintainers will get informed that there will be serious breaking changes with CMS 10.The question is, how we can reach out to extension maintainers to collect the necessary information. Current comments like
[...]
are not that helpful, since they just tell people "don't use it, since it's none of your business", which is wrong regarding the situation with hooks, but they don't encourage communication about the necessary variables. So maybe there is another way to get people involved.
So you're saying, the text should be adapted to "... Is intended for internal storage only. If you feel differently, reach out and open up a ticket on forge.typo3.org" ?
Updated by Jo Hasenau over 5 years ago
How about
... Is intended for internal storage only. If you really need to access it from outside this class, reach out and file an issue on forge.typo3.org to request a method.
?
Updated by Benni Mack over 5 years ago
Sounds good, can you provide a patch for that?
Updated by Christian Welzel almost 5 years ago
What is the status of this issue?
This bugreport was about TYPO3 9 and in the meantime TYPO3 10 was released without the access to the properties $id and $colPos.
What is the proposed way to retrieve this information inside the Hook?
I found the global variable $GLOBALS['TYPO3_REQUEST'], which is used through the whole core, but it is deprecated since 9.2.
Is there an other option besides accessing $_GET?
Updated by Gerrit Code Review almost 5 years ago
- Status changed from Accepted to Under Review
Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/62480
Updated by Gerrit Code Review almost 5 years ago
Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/62480
Updated by Gerrit Code Review almost 5 years ago
Patch set 1 for branch 9.5 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/62457
Updated by Jo Hasenau almost 5 years ago
- Status changed from Under Review to Resolved
- % Done changed from 0 to 100
Applied in changeset f7234b8a56a3864fde2b613e8f7ceea03b1aa854.
Updated by Benni Mack almost 5 years ago
- Status changed from Resolved to Closed