Bug #87064

Deprecated public properties can't be accessed after removal

Added by Coders.Care Extension Team 8 months ago. Updated 4 months ago.

Status:
Accepted
Priority:
Must have
Assignee:
Category:
Code Cleanup
Target version:
-
Start date:
2018-12-03
Due date:
% Done:

0%

TYPO3 Version:
9
PHP Version:
7.2
Tags:
Complexity:
Is Regression:
No
Sprint Focus:

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);
and then processed like 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.


Related issues

Related to Grid Elements (former official tracker) - now moved to Gitlab! - Bug #85441: Notices when opening a CE gridelement (non-cached) Resolved 2018-06-30

History

#1 Updated by Coders.Care Extension Team 8 months ago

  • Description updated (diff)

#2 Updated by Coders.Care Extension Team 8 months ago

  • Description updated (diff)

#3 Updated by Georg Ringer 8 months 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

#4 Updated by Coders.Care Extension Team 8 months 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.

#5 Updated by Coders.Care Extension Team 7 months ago

  • Related to Bug #85441: Notices when opening a CE gridelement (non-cached) added

#6 Updated by Georg Ringer 4 months 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!

#7 Updated by Jo Hasenau 4 months 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.

#8 Updated by Benni Mack 4 months 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

#9 Updated by Jo Hasenau 4 months 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.

#10 Updated by Georg Ringer 4 months 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

#11 Updated by Benni Mack 4 months 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.

#12 Updated by Jo Hasenau 4 months 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.

#13 Updated by Benni Mack 4 months 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" ?

#14 Updated by Jo Hasenau 4 months 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.

?

#15 Updated by Benni Mack 4 months ago

Sounds good, can you provide a patch for that?

#16 Updated by Jo Hasenau 4 months ago

  • Assignee set to Jo Hasenau

Also available in: Atom PDF