[TYPO3-team-core-v5] Comments about commits during my holidays ...

[Robert Lemke]

Hi folks,

I spent this morning with going through all changesets you merged during
my holidays. In general I loved to see how much you progressed and I
like the new features you introduced. But it wouldn't be me if I didn't
have some comments and picky rants ;-)

Please go through the list shortly and if you were the author of a
changeset, I'd be glad if you could check out if my comment makes sense
and let us know if you're going to tackle it or not.

Some general note: Please be careful not to introduce so many inline
comments. As you know it's almost always a bad code smell. If the code
doesn't get clear enough, think about extracting it into its own method.
If the code is obvious enough (like here
https://review.typo3.org/#patch,sidebyside,1969,7,Classes/MVC/Web/Routing/Router.php),
don't write an inline comment. If it's some general point you can
explain in the method's doc comment, please do so. And if after
considering all this you still feel like adding an inline comment, do so.

About the command argument support: I'm happy that Christopher
implemented it! You couldn't know that I had started doing that as well
on my local machine – but fortunately it wasn't that much code I created
yet. However, I did have a little different concept in mind which would
additional allow to do this "./flow3 flow3:package:create Foo" instead
of this "./flow3 flow3:package:create --package-key Foo". In any way,
that will certainly be easy to incorporate afterwards.

Now, here are the changesets (in reverse order):

*** FLOW3 PACKAGE ******************************************************

May 24 2011 TASK] Run Doctrine schema and proxy setup only if needed
Change-Id: I846fc4c7eff4523947bb17f3963e518390547199

I don't think that the CacheManager is the right place for
markDoctrineProxyCodeOutdatedByChangedFiles(). This slot should rather
be in a class of the Persistence subpackage or if there's no nice place,
put it into the bootstrap for the time being (there are other slot lying
around already).

----------------------------------------------------------------------------------
May 19 2011 [+FEATURE] Add a typo3.org SSO authentication provider
Change-Id: Iffba555c5b4e949c4b61b5f4c5360e2bf6a95b4a

The SSO authentication provider is great, but does it really belong to
the FLOW3 package? It's very specific to TYPO3 / typo3.org and I plead
for moving it into a dedicated / other package.

----------------------------------------------------------------------------------

May 18 2011 [+BUGFIX] (MVC): Original Request must only be set if there
is a validation error
Change-Id: Ic2179e21b97da9ad3cd4e5685f85e961c36edc63

In the commit message you write "Then, the "Speaker Profile" of the
conference site is correctly filled out again.". Imagine that in the
changelog / release notes of the FLOW3 distribution – doesn't make a lot
of sense. Also this changeset cries for a unit test because the logic /
motivation behind setting the original request could be explained by
that test and secured at the same time.

----------------------------------------------------------------------------------
May 16 2011 [+BUGFIX] (MVC): Introduce internal Request Arguments
Change-Id: Ic0c7a7abbd9cb26de2916acf81bf0a523d9c762a

Why do the internal arguments "__referer" and "__CSRF-TOKEN" have a
different case? Why not "__csrf-token"? or "__csrfToken"?

----------------------------------------------------------------------------------

May 16 2011 [+BUGFIX] (Object): Session objects with fallbacks
Change-Id: I55d8ad331117cdeff62e6ddf0cce0153499879bc

While this patch fixes some bug, from the information given I cannot
reproduce it. It's just mentioned that in rare cases it can happen. A
unit test instead of the inline comment would be better.

----------------------------------------------------------------------------------
May 08 2011 [FEATURE][BUGFIX] Calculate referenced column names from
class code
Change-Id: If2a4612a326b6fbdfa4a27363c491723ae5cf1e7

Karsten, you're using "_Original" in some regexes there. There currently
is a static property in \F3\FLOW3\Object\Proxy\Compiler which defines
that string. We either remove that (and thus say we won't change it
anytime soon again) or use should refer to that variable.

----------------------------------------------------------------------------------
April 26 2011 [TASK] Use native serialize for session scope objects
Change-Id: I6b7e17806196f82c5744050cf5d15b7f081416c4

In this changeset from end of april Andi introduced two new methods to
the DI ProxyClassBuilder:

https://review.typo3.org/#patch,sidebyside,1485,12,Classes/Object/DependencyInjection/ProxyClassBuilder.php

While I agree that this functionality is needed, I think that the DI
ProxyClassBuilder is simply the wrong place to add it. It doesn't have
anything to do with Dependency Injection (as far as I understand) and
especially the code introduced in buildSerializeRelatedEntitiesCode() is
almost too long to be introduced in an inline string. Can't this be
introduced by some advice?


*** TYPO3 PACKAGE  *****************************************************

May 17 2011 [+BUGFIX] (UI): Fix JavaScript error when party.name not set
Change-Id: I120e65aaa3b4b0ec7bbd70a431e6850cb2c8429d

As far as I can see the SetupCommandController _does_ create a Party
(User) for the created account - so was there maybe something else go wrong?

----------------------------------------------------------------------------------

May 26 2011 [TASK] Fix location of image in error page
Change-Id: I201362fe27987dcbc8df653db210cf8468a4d8a5

Shouldn't the URI builder create the right link to the images even
without the "package: 'TYPO3'" argument because "TYPO3" is the current
package? A bug?

*** TYPO3CR PACKAGE  ***************************************************

May 17 2011 [BUGFIX] ProxyNode only persists the newNode if it has not
been cloned
Change-Id: I93eaddf0131d1c7ab8ca29108d87976063f0951b

As already mentioned, there are way to many inline comments for my taste.