Save custom objects using custom doctrine type mappings

[Stephan-Kok]

Instead of the generic types “array” and “object” we have created custom Doctrine types that will handle the conversion to and from database value and PHP value. With implementing this change the database is more adaptable to possible changes, is more readable and it is usable by other languages than PHP.

In the current pull request we have only adjusted on how these types are saved and read from the base, there have been no functional changes.

Work that remains to clean the database further contains the following items:

• X509CertificateLazyProxy should not take factory from the constructor, but should rather create its own factory when initialized.
• Organization names (nl / en / pt) should not be default be created without any values. Currently the database contains many ‘{"name":null,"displayName":null,"url":null}’ values, these should be replaced with null values
• allowed_idp_entity_ids should be of type string[] as defined. Currently the object is saved as both array and dictionary.

This pull request requires a migration of the database. Since values are stored and retrieved in a different manner, the table sso_providers_roles_eb5 needs to be emptied and filled with new values (Aka push from Manage).

Take special note to column ‘logo’ in table ‘sso_providers_roles_eb5’ since ‘nullable=true’ was missing from the declaration in abstract.php it is likely that the column contains a NOT NULL constraint. This needs to be removed.

[thijskh]

Thanks for the PR. It does seem that this would break a running EB instance. We do not deploy releases big-bang but gradually roll them out. Any data structure changes would therefore need to be implemented in a backwards compatible way, at least for one release.

[Stephan-Kok]

Hi Thijs, thanks for all the feedback.

As discussed we will split the PR in 3 stages where each PR should be accepted in a new release. As discussed there will be some overhead during a push from manage during the first two releases (since we push to two tables). But these approach implements the changes in a backwards compatible way for one release.

The required steps per release are:

• prepare eb6 - Introduce sso_provider_roles_eb6 and temporary push to both sso_provider_roles_eb5 and sso_provider_roles_eb6. Still use sso_provider_roles_eb5 for the main process
• switch to eb6 - temporary push to both sso_provider_roles_eb5 and sso_provider_roles_eb6. Use sso_provider_roles_eb6 for the main process
• Remove eb5 - Clean up and remove sso_provider_roles_eb5

This PR will contain the the first step prepare eb6. I will create the two additional PR's with the other two steps.

Notes:
This PR contains a database migration

[Stephan-Kok]

I have updated to tests, to also write and reset the new table 'sso_provider_roles_eb5'.

Please see the the additional PR's in:

• prepare eb6 - Save custom objects using custom doctrine type mappings #1159
• switch to eb6 - Save custom objects using custom doctrine type mappings switch to eb6 #1166
• Remove eb5 - Save custom objects using custom doctrine type mappings Remove eb5 #1167

[MKodde]

Hi @Stephan-Kok Let me start by expressing my thanks for all the work you guys are doing at Kenisnet for EngineBlock!

I've been off the EB project for a couple of months and have focussed my attention on the StepUp project in the meantime. I've been watching some of the work here with interest.

In this PR you are moving forward with expanding the way the 'roles' are stored, utilizing the Doctrine mapping type system to serialize/deserialize the entity metadata.

Personally I'm not a big fan of this route, as we adhere to Doctrine more and more. But I know we are already heavily invested in Doctrine and the mapping types so utilizing it for this changeset is a logical decision.

A (slightly) more pressing concern are the metadata objects that are stored in the database. The metadata objects are not type-safe. They are like this by design (I feel we should review this design in the future and maybe move towards a more safe immutable value objects based approach). Have you zoomed in on this while building the doctrine types?

There is a chance that the conversion between the PHP data and the data actually stored in the database will result in conversion issues (value truncation) that are handled silently by the DBMS driver. I recently fixed a similar issue where the NameID Doctrine Type did not consider the VARCHAR length set in one of the Doctrine Migrations, truncating the field in the table field. With most default Mysql/MariaDB configurations this only produces a warning in the logs, but truncation does occur. Opening possible attack vectors for malicious users. I have not studied if any of your changes have this vulnerability. By the way, fixing this issue can be as simple as configuring the DBAL driver to be more strict in it's truncating operations. But having more substantial input validation in the Metadata objects might also be a good thing to start investing time and effort in. Again, this problem is not easily solved, and might require some additional drawing board action and refining.

In the mean time my I suggest that you create static constructors on the Metadata objects you are using (creating) in the Doctrine Types? As an example you could look at the Coin type we added some time ago. This allows you to at least type check and input validate the data that is set initially on the object. Also, feel free to use the PHP 7 type checking features. We no longer support PHP 5, so we can start adding parameter type definition and return type definitions.

I'm very grateful for you adding the unit tests for the *Type classes!

[Stephan-Kok]

As discussed:

I have created the static constructors for the custom types. Hereby any validations can be added to the constructor of the Entity it self. Since the goal of this PR is a refactoring of the database types I have not implemented any additional validation (since that would be a functional difference). If I am correct, this issue of already missing validation is discussed internally and (possible) put on the backlog.

Additionally I have created some constructors that were missing. Do note that I have not updated the missing constructor creations in different parts of the code. This is because I want to touch as less code as possible with this PR. This also brings me to the question of when you think we would be able to create these 3 releases, since I would have to merge all of the new and upcoming changes back in the PR before we can continue I would like to be able to do this a soon as possible.

[thijskh]

I'm a bit confused by the three PRs. It seems they cumulate all changes.

• prepare eb6 (Save custom objects using custom doctrine type mappings #1159 - this PR) is the base
• switch to eb6 (Save custom objects using custom doctrine type mappings switch to eb6 #1166) seems to include all changes from 1159 plus the switch?
• remove eb5 ( Save custom objects using custom doctrine type mappings Remove eb5 #1167) seems to include all changes from 1159 plus the switch plus removal of the old ones?

[MKodde]

This PR was parked for a while now. And other work to the Roles have been done. The following should help in:

MDUI changes are not yet included and should be.

Fields marked for deprecation (in master) and already replaced by the MDUI composite field:
• $descriptionNl $descriptionEn $descriptionPt
• $displayNameNl, $displayNameEn, $displayNamePt
• $logo
• $keywordsNl, $keywordsEn, $keywordsPT

Discussion point: This would be a great opportunity to introduce multilingual fields for the other, now hard coded, multilingual fields.
• $nameNl, En, Pt
• $descriptionNl, En, Pt
• $organizationNl, En, Pt

Update on discussion point above:

IRL during the OC hackaton we discussed this point. We want to go forward with including these changes to have multilingual fields for the name, decription and organization. But we do not want to be hung up on this at this point.