BUGFIX: Allow to inherit Accounts after the merge of #2700 again

[fcool]

Upgrade instructions

Neos Setup should work now again

Review instructions

See inline comments - they should explain the idea of this change.

This solves https://github.com/neos/neos-setup/issues/11

Checklist

• [x] Code follows the PSR-2 coding style
• [x] Tests have been created, run and adjusted as needed
• [x] The PR is created against the lowest maintained branch
• [ ] Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
• [ ] Reviewer - The first section explains the change briefly for change-logs
• [ ] Reviewer - Breaking Changes are marked wit !!! and have upgrade-instructions

[bwaidelich]

to inherit Accounts for…

...for what? :)

[fcool]

Lets make it short. ;)

It's not possible to inherit Accounts in general.

Of course that will never be possible, as long as the Accounts have no Doctrine inheritance type, but an inheriting class is used for example in neos/setup, which uses the inherited account class as sessionless account without any persistence to carry extra data.

This pattern has been reported to be quite "regularly" used. (It is used for example by neos/setup and even one of my projects copied that pattern, I just detected during the last maintenance and updates ;) )

So this should only solve the biggest pain - keeping the possibilities, which had been there as long as "isNewObject" was broken.

[fcool]

D'oh... the one failing test is probably right... missed to run the complete suite. I'll have a look, if this can be solved. (Even if the result is not that different - its a different exception now ;) )

[fcool]

Replacing get_class with TypeHandling::getTypeForValue solved the issue with the tests.

[fcool]

I'm surprised... in Flow neos/setup now works again - and it self never calls this not longer existing method. And I did not find any usage in a Neos 7.3, too.

How exactly does your test setup look like?

[crydotsnake]

I installed the base distribution and copied your change over. Maybe this is the wrong way to test it but i'm sure this should also work 😅

[fcool]

Neos or Flow base distribution?

And which branch of the base distribution?

But yes, your "process" should work.

[crydotsnake]

Neos.

Its a fresh installation with composer:

composer create-project neos/neos-base-distribution

[fcool]

Oh sorry. It looks like I did my first test in a system, which was not affected at all - sorry.

Following the procedure from @crydotsnake I discovered the real problem is here, that Neos.Setup is using Flows FileBasedSimpleKeyProvider which itself creates an account on its own. Which will now bring the PersistenceManager to make a lookup before the DB was set up.

But of course, now having a fix for the "pattern", which is not exactly used here, a possible fix is extra easy. (I added a new AccountWithoutPersistence for this case, and as such it would work...

But if its really only flow, maybe we should create another Interface which objects should implement to get not checked for their "newness"... (Just thinking)

Maybe this here, and a new interface here too and removing that spooky guard in 9.x again?

[crydotsnake]

Hm. I applied your changes but still get the same error message as before.

Call to undefined method Neos\Flow\Persistence\Doctrine\PersistenceManager::persistAllowedObjects()

[fcool]

That's really strange for at least two reasons:

1. Why can't I reproduce this issue (I tested with PHP 8.1)
2. The method is there - there is no chance to get a undefined method at this class and this method.

[crydotsnake]

I'm also not sure.

Maybe the way how i test this is not the best way with copying the changes over. But i checked everything a few times so i definetly didnt forget too copy the changes.

[crydotsnake]

How do you test this? i normally try too login to the setup tool. And i also use PHP 8.1

[fcool]

Thats my Testscenario too. :P

How do you copy the changes? (I usally take the patch from here and apply it there or directly check out from my own "remote", after adding the flow-development-collection to the base distribution)

[crydotsnake]

Thats my Testscenario too. :P

How do you copy the changes? (I usally take the patch from here and apply it there or directly check out from my own "remote", after adding the flow-development-collection to the base distribution)

I go too the files you changed and copy all its content over. So basically nothing can go wrong here.. I will setup a new neos-development-collection based on the 7.3 branch und just switch to your branch with git.

[mhsdesign]

@crydotsnake the setup is broken beyond repair. Thats why we are working on replacing it ^^

I would +0.5 this pr, because the pattern is used in other projects but not because it fixes neos/setup :muhahaha: - and i dont understand the code ^^

[fcool]

We have to support 7.3 and 8.3 for the next years. So even if I'm totally with you, that it should die - we simply cannot drop it from already released versions without breaking our own guarantees. So we HAVE to repair it for the versions, that where released with it. (At least IMHO)

It is "without persistence" because each Object using Persistence need to be annotated as "Flow\Entity", "Flow\ValueObject" or "ORM\Entity" - and exact this annotation missing (needing at least one of them) is, what makes the Object "without persistence"). As Doctrine knows nothing about them (ClassSchemas get only Build if they are configured entities. Extending Account is doctrine wise impossible, because the because class does not declare an inheritance scheme.)

And apart from that: I'm not that pessimistic. In Flow the Setup process is already fixed with this single commit. "Beyond repair" should be more work than a simple fix.

[bwaidelich]

Hi all, I didn't follow the discussion but just stumbled upon some new AccountWithoutPersistence class in this change (which does not seem to fit in a bugfix IMO). Also are you aware of the whole AccountInterface undertaking (see https://discuss.neos.io/t/rfc-accountinterface-and-authentication-rework/4966 and https://github.com/neos/flow-development-collection/pull/1939).

I still hope that someone picks up this work at some point

[fcool]

@bwaidelich : The mentioned PR is something I'd love to attack as soon as my evergreen (doctrine) is sorted. Regarding your other concern - in theory I'm with you. But fixing the persistence manager broke a pattern, which worked before. And every solution I can think of would involve to introduce a new marker in some way (i.e. a new annotation, a new interface, a new class).

And we need the "now" to be working now. Of course it makes no sense to introduce new features like that... But the only alternative would (probably) be to return to the broken state or deliver even in Flow itself broken code (the FileBasedSimpleKeyProvider is not usable currently).

So we need to move - even if it is not ideal.

The complete Account rework can never target 7.x or 8.x, so we need some "interim" solution.

[mhsdesign]

@fcool i unblocked it from my view but as I’m not super deep into it I can’t agree either ;) (but I would like to discuss this with you maybe in the next teammeeting - unless someone else reviews the code ^^)

[sorenmalling]

So we need to move - even if it is not ideal.

The complete Account rework can never target 7.x or 8.x, so we need some "interim" solution.

Could we name it TransientAccount, so we build on top what could come?

https://github.com/neos/flow-development-collection/pull/1939/files#diff-5c52a2da4d71ad6dbcb5cdc38d63b507fd04199d0790a65c074ad038eb11dc03

[fcool]

I'm not merried with the proposed name. In fact, it's the best I could think of, but I'm not lucky with it either.

But I must admit that I'm strongly against transient here. Because Transient for Flow means: "No persistence AND no session". And specifically this account is built for the session, so the FileBasedSimpleKeyProvider can work again. As it is NOT sessionless.

[fcool]

Okay. I setup a new neos-development-collection and could now login too the setup tool. But now it fails at the last step where i can import the Neos.Demo package:

20230707142547fa8fd9.txt 20230707142547531aae.txt

Exceptions are attached.

@crydotsnake: That cannot be solved here. That is a bug in the Neos.Setup - Step of Neos - the other development collection ;)

[crydotsnake]

Are you sure? In 20230707142547531aae.txt

it says:

Object of class Neos\Flow\Security\Authentication\AccountWithoutPersistence could not be converted to string

Also this bug never occourd before.

[fcool]

I followed the Neos setup this time.

The error originates from PartyService::getAssignedPartyOfAccount which calls PartyRepository::findOneHavingAccount with the "SetupAccount" and there a query is generated - which now cannot be serialized, as the new Account class is unknown to doctrine. Of course this method wouldn't find an assigned party anyway. ;)

I see two "easy" possibilities to get around this: We could add a __toString to the "Stupid-Account"-Class (that way doctrine can generate a valid SQL), or add a "PersistenceManager::isNewObject" call to the PartyRepository::findOneHavingAccount method and skip the query if the answer is true (that wouldn't change anything, as not persisted accounts will never be found in the database).

Unfortunately we cannot use anonymous classes - I thought the idea would be nice, as it would allow us to get around this trouble without creating new classes... But that won't do, as anonymous classes cannot be serialized, which is crucial for the session to work.

[crydotsnake]

Okay. I setup a new neos-development-collection and could now login too the setup tool. But now it fails at the last step where i can import the Neos.Demo package:

20230707142547fa8fd9.txt 20230707142547531aae.txt

Exceptions are attached.

After merging this pull request we should definetly take a closer look in to this. I will test this on a fresh neos-development-collection if this exception occours again.

[fcool]

Read my last comment - I see two possible solutions for those Exceptions. ;)