[TYPO3-core] RFC: #11438: Add a registry to t3lib

[Ingo Renner]

Oliver Hader wrote:

Hi Olly,

thanks for the sunday review :)

> Some remarks to the patch:
>
> * database
>    + field "uid" is never used so why must it be a primary key and
>      defined at all?

The uid field exists for the sake of consistency and I also remember 
someone (Dmitry?) once having said a numerical PK would be faster than a 
literal one. However, I was thinking about that too already. So let me 
know whether this should be changed.



> * method __construct()
>    + is it required to have everything from the registry table loaded?

Yes, this way extension A can access settings from extension B or the 
core, too. I also do not expect to have a large registry as with Windows ;).
Also notice that the class is a singleton, so that SELECT * is executed 
only once.

>      there could be a method "getByPrefix('tx_whatever')

This could be an additional method indeed, but to me it would be an 
addition to get(), thus returning an array of values.

>    + please set "$this->entries = array();"

Where?

> * method get()
>    + is it required that every variable must have a value?

Yes, otherwise it wouldn't make a lot of sense. When asking for a value 
you actually want to have it, otherwise you wouldn't make the call. 
Then, with the default value you can be sure to always get "something" back.

>    + the $defaultValue should be optional and set to NULL if not defined
>      otherwise undefined variables are not possible with the new registry

No, see point above.
For the null case, simply set null for the second parameter. If the 
default value were optional and set to null by default, you'd always 
have to check for the retrieved value not to be null. By requiring a 
default value you save that check.


> * method set()
>    + the allowedNameSpacePrefixes should be a class member variable

Why, it's the only place it's used.

>    + as far as I see, for core variables only stuff like "core.whatever"
>      should be possible - but not "coreSomeThing.whatever" - but the
>      validation against allowed prefixes does not catch this

I've changed the core prefix to 'core.'

>    + the check against a dot in the key whould also allow this "tx_test."

Well yes, but how much sense would that make? If you do that, you either 
exactly know what you're doing, or you probably don't ;)

In general the check for namespacing is not intended to be super strict. 
The checks as implemented now should be enough for most cases.

>    + sql_affected_rows() only returns a positive integer if the database
>      record was modified - thus, updating a record without changing the
>      data would result in calling an INSERT query and running into a
>      "duplicate key" MySQL error in this case

Hmm, true. On the other hand, why would you want to set an entry that 
didn't change? Otherwise, what would you suggest to resolve this and 
still keep the code simple?

> * test cases
>    + the registry is used in all tests - thus create $this->registry in
>      the setUp method

shouldn't change anything due to the registry being a singletone, 
changed anyways.

>    + IMO writing test data to a live database on running the tests is not
>      a good idea. It would be better to store data to a separate database
>      or table. AFAIK there's the database testcase in tx_phpunit...

I'm not that much into phpunit. Could you do these changes, please?


Xavier's point about escaping entry keys has been taken into account, too.


best
Ingo

-- 
Ingo Renner
TYPO3 Core Developer, Release Manager TYPO3 4.2