Segregated support of unique index and unique constraint

[guilhermeblanco]

Added missing commits:

• 400d73c
• e91811c
• a4ecf05
• d7d1c39
• c71b3ae
• 2f9da39
• 9aa4e79

BC BREAK: Doctrine\DBAL\Schema\Table constructor new parameter

Deprecated parameter $idGeneratorType removed and added a new parameter $uniqueConstraints.
Constructor changed from:

__construct($tableName, array $columns = [], array $indexes = [], array $fkConstraints = [], $idGeneratorType = 0, array $options = [])

To the new constructor:

__construct($tableName, array $columns = [], array $indexes = [], array $uniqueConstraints = [], array $fkConstraints = [], array $options = [])

[lcobucci]

@morozov should be the one checking this. However, as it was mentioned on Slack, I'm pretty sure you should be looking at using DBAL 4.x instead of 3.x

[guilhermeblanco]

@lcobucci unfortunately the amount of work for dev-master is way too big. I need first to make it work back again (for 3.0.x), and then I can consider pointing to dev-master (4.0.x).
As an example, there were lots of API breakage that would make this an herculean task to do in one shot. For example, Query->setParameter() broke the API by accepting strings for type, which prevents the support for setting arrays now (ie. Connection::PARAM_INT_ARRAY, etc), and will make the sync work much harder than it needs to be for now. My effort is to finish ORM 3 work, so we can reiterate for ORM 4.

[SenseException]

• Can we remove underscore prefix in method and property names at least in new classes?
• Since this is a major release branch that had no release yet, can return values be added to methods (at least those with scopes less than public)?
• As mentioned in a comment it looks like there are missing tests.

[guilhermeblanco]

• Can we remove underscore prefix in method and property names at least in new classes?

There is only one class added to project (UniqueConstraint). I've fixed the only method that was using underscore.

• Since this is a major release branch that had no release yet, can return values be added to methods (at least those with scopes less than public)?

I've moved a lot of methods following logical declarations. I only added 2 public methods and added type hint and return types there.

• As mentioned in a comment it looks like there are missing tests.

Fixed that adding tests to AbstractPlatform::getSequencePrefix()

[SenseException]

Looks good. 👍 Only the coding style issues seem to prevent a full build.

[morozov]

Since this is a new API and a breaking change, I'd expect thew new API to be documented in the PR description and the BC break documented in upgrade notes.

When we decided to release DBAL 3.0 earlier for compatibility with PHP 8, we agreed that it should only have trivial to handle breaking changes. There doesn't seem to be much BC break here, so it should be good IMO.

@guilhermeblanco please make sure all new methods have native parameter and return type declarations.

[guilhermeblanco]

@morozov all updated. Please merge if appropriate.

[morozov]

This looks like a leaky abstraction to me. Especially because it's not used by DBAL itself. Given that all the internally used APIs are public, should ORM implement this logic instead?

[guilhermeblanco]

ORM is the one that would be leaking abstraction if calling this method.
Conceptually, ORM is telling DBAL "build me a primary key sequence for this table under this schema". The fact that ORM needs to do more than that is a bad abstraction on its own.
DBAL should defer to platforms that once a PK sequence is requested, a sequence named "bar__foo_seq" should be created if schemas are not supported or "bar.foo_seq" if they are.

I migrated the method to the project that owns it. A later stage would be encapsulate it and handle it accordingly internally without exposing this API.

[morozov]

ORM is the one that would be leaking abstraction if calling this method.

As far as I understand, what you're saying is ORM already relies on implementation details of DBAL. This is a given fact and it cannot be used to make DBAL leak even more of its implementation details.

The very fact that there's a string in ORM that "bar__foo_seq" is built with "__" defined DBAL and "_" defined in ORM looks horrible.

I would agree that the DBAL abstraction used by ORM needs to be improved but the fact that it's already bad cannot be used as a reason to make it even worse.

[morozov]

The addition of Platform::getSequencePrefix() is really questionable. Although this code is ported from the former develop, I don't see any PR/discussion through which it would get merged there.

[guilhermeblanco]

@morozov answered you in the specific comment about why it was moved.

[morozov]

@guilhermeblanco given the lack of progress on this issue in the past months and the fact that we're approaching the release, I'm inclined to move it out of the release milestone.

Please let me know if you plan to finish the work before the release.

[vv12131415]

@morozov it's there

dbal/src/Platforms/AbstractPlatform.php

Line 3110 in 5c7a0e2

public function getSequencePrefix(string $tableName, ?string $schemaName = null) : string

AbstractPlatform::getSequencePrefix()

[morozov]

I don't see getSequencePrefix() being part of a public DB abstraction API.

I mean that I don't agree with this method being introduced.

[vv12131415]

@guilhermeblanco can you please rebase? So that version 3.0 can be released.

[guilhermeblanco]

I'm on it... should be ready later today or tomorrow

[morozov]

@guilhermeblanco do you need help with this?

[guilhermeblanco]

I worked on that yesterday, but the amount of caused conflicts (and their pointed blocks) are too hard to resolve.
I feel the easier approach seems to be cherry pick the commits on top of current 3.0.x branch.
If you have time, feel free to start. I can wrap it up tonight if you can't make it to the end.

[morozov]

@guilhermeblanco I rebased this and reorganized the commits in the way that each individual commit passes phpcs thereby eliminating commits like “Fixed code style”.

Please make the needed code changes and do some more cleanup. E.g.:

1. The “Normalized parameter name across platforms” is no longer relevant since the same was done in Clean up inconsistent or redundant argument names #4172 and Manually merge 2.11.x into 3.0.x #4224.
2. Commits like “Type hinted Table constructor” and “Added missing return method” are not really relevant from the code change history and can be combined with others.
3. If “Moved back methods” revert the changes made in the same PR, it's better to not introduce these changes in the first place in order to not cause potential merge conflicts.
4. Normally, each commit should pass all build stages in order to be really atomic. This can be accomplished via git rebase -i upstream/master exec $cmd where $cmd is phpcs, phpunit, phpstan a, psalm, etc. This will help to bisect potential issues and not deal with knowingly broken states. If you don't want to do this, please squash everything.

In the future, please open PRs from your personal repo instead of this one. Otherwise, the PR gets built twice on each CI platform which slows down the feedback.

[morozov]

@guilhermeblanco given your lack of availability, how do you think we proceed here? Should I just remove getSchemaPrefix() and count that you handle it later in the ORM or you wanted to make more changes? Note that we're already 3 weeks behind the schedule and it makes us maintain an extra 2.10.x release.

[SenseException]

I'm fine with the changes. If getting getSequencePrefix() moved back to ORM is the last step of this PR that needs to be done, then I'm 👍 with this PR and it can be merged after that.

Handling getSequencePrefix() in DBAL can be done in a separate PR in coordination with an ORM PR.

See #3980 (comment).