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 neos/neos-setup#11

Checklist

• Code follows the PSR-2 coding style
• Tests have been created, run and adjusted as needed
• 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.

[kitsunet]

Seems like a fair comrpomise for the time being? Might still be weird for some stuff, but well..

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

[crydotsnake]

Hm. I tested it but now i get:

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

I dont know if i do something wrong

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

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

[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 ^^

[mhsdesign]

hmm i dont understand this 😂 why is it without persistence? doesnt it just inherit?

If the change here and the changed FileBasedSimpleKeyProvider is only to fix the setup (a pseudo fix, because then it will fail at another point - sorry for being so pessimistic but im spending a lot time into the new alternative neos/neos-development-collection#4243 so i just want it to die) i rather have the code in the neos/setup package with an extended FileBasedSimpleKeyProvider so i can burn the code with fire once the new setup is ready.

Putting code like this AccountWithoutPersistence directly into flow will never allow us to get rid of it ^^ as its seemingly api no? - also we wont likely going to clean it up after the old setup is finally dead.

[sorenmalling]

Is this the TransientAccount that we initially created here ??

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

This patch also creates a solution for the FileBasedSimplyKeyProvider.

I will love that we solve the AccountInterface task before creating solution around the real issue at hand

[fcool]

@sorenmalling : We won't merge this in "past context" - so in 7.x or 8.x. Yes the branch would be MUCH better to have merged. But as it touches many places and is at least at one place breaking if I see it correctly, we simply cannot merge it in released versions.

So our goal is to have it in as soon as possible - ideally 9.0, 9.1 at latest.

But this won't fix the problems already "out".

[albe]

Late to the game, but two things:

• isn't this class gonna receive a schema still, since it inherits from an entity?
• as far as I understood, this is supposed to be a hotfix intermediate class, that will vanish or be replaced. Let's do everything to make this super clear and prevent anyone from using the class outside the FileBasedSimpleKeyProvider. So make the name annoyingly explicit about it, and add a doc block stating this, potentially also directly annotating it as @deprecated

[mhsdesign]

what about using an anonymous class? Would that work? (Ignore my comment if its totally useless 😂 )

[fcool]

I have already tried that:

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.

[fcool]

@albe:

• No it becomes no schema, because the Entity annotation does not "inherit".
• Yes, if we think this "approach" is acceptable at all, we relly should make clear, that this is only an "interim" solution. And we really should take care to merge the better account semantics at least for 9.0

[mhsdesign]

Sorry i have to ask about https://github.com/neos/flow-development-collection/pull/3109/files#r1255771785 ^^

[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 #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.

This is the way

[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. ;)