Adds default namespace to schema. #2490

garex · 2016-08-26T14:23:31Z

Adds default namespace to schema

Some databases supports namespaces and has default one. For example Postgres has default "public".

When schema created from classes, which has table names without namespaces, but this namespace is assumed, we should add it explicitly, if it not exists yet.

It will help in diffs between real DB and config shemas.

The visitor is skipping platforms that don't support schemas.

This is 1st part of fix DBAL-1168 #1110

Some databases supports namespaces and has default one. For example Postgres has default "public". When schema created from classes, which has table names without namespaces, but this namespace is assumed, we should add it explicitly, if it not exists yet. It will help in diffs between real DB and config shemas. This is 1st part of fix DBAL-1168

garex · 2016-08-26T14:26:40Z

Next step will be to add this visitor to the end of Doctrine\ORM\Tools\SchemaTool::getSchemaFromMetadata

    $schema->visit(new AddDefaultNamespace($this->platform));

Ocramius · 2016-09-07T21:41:18Z

lib/Doctrine/DBAL/Schema/Visitor/AddDefaultNamespace.php

+ */
+class AddDefaultNamespace extends AbstractVisitor
+{
+


What wrong with it?

It's missing :-)

It's missing :-)

What is missing?? There is top comment and class comment. What is "docblock" then? Can you provide an example?

/** * @var AbstractPlatform */ private $platform;

Ocramius · 2016-09-07T21:42:59Z

Next step will be to add this visitor to the end of Doctrine\ORM\Tools\SchemaTool::getSchemaFromMetadata

Could be added by default, but then act as no-op if the platform doesn't support schemas?

garex · 2016-09-08T03:26:38Z

Could be added by default, but then act as no-op if the platform doesn't support schemas?

I also thought about this option first, but then deside to do the check outside to give more readability at the place where it should be added at getSchemaFromMetadata.

But if your experience tells you to move it inside -- let's move it inside. It will become more reusable then.

Ocramius · 2016-09-08T12:29:43Z

If it's not a no-op when the platform does not support schemas, then it will become a setup issue for most users, making working apps not portable. IMO, it should be done inside the visitor.

garex · 2016-09-08T13:09:28Z

Ok, I will rework it. Pls not merge many commits, I will rebase em after approve.

Regarding docblock. What bad there? Bad @since or description?

Ocramius · 2016-09-08T13:11:27Z

lib/Doctrine/DBAL/Schema/Visitor/AddDefaultNamespace.php

+ * It will help in diffs between real DB and config shemas.
+ *
+ * @author Alexander Ustimenko <a@ustimen.co>
+ * @since 2.5


--- Branch will be rebased after approve --- Merge branch 'master' into DBAL-1168-add-default-schema-name

garex · 2016-09-09T04:00:51Z

Pls give me know when you think it will be ready for merge -- I will rebase it. Or you can rebase it on master and squash all in one commit.

garex · 2016-09-12T12:42:58Z

@deeky666 when approximately will you plan to review?

garex · 2016-09-23T09:31:25Z

@Ocramius please assing someone else or review by yourself.

Two weeks passed already and @deeky666 is not reponds.

phansys · 2016-09-26T00:38:28Z

lib/Doctrine/DBAL/Schema/Visitor/AddDefaultNamespace.php

+{
+
+ /**
+ * @var \Doctrine\DBAL\Platforms\AbstractPlatform


This class is already imported, so you should use @var AbstractPlatform instead.

Not sure. In other classes in same directory also fully qualified names used. So I decided to leave it as it was here before.

phansys · 2016-09-26T00:40:28Z

lib/Doctrine/DBAL/Schema/Visitor/AddDefaultNamespace.php

+ */
+class AddDefaultNamespace extends AbstractVisitor
+{
+


Please, remove surplus LF.

--- Branch will be rebased after approve --- Merge branch 'master' into DBAL-1168-add-default-schema-name

garex · 2016-09-29T17:57:00Z

This PR is covered with cobwebs. Betting, it will be merged may be after New Year.

Ocramius · 2016-09-29T18:56:15Z

There's waaaaaay older stuff around here. If @deeky666 didn't have time to check it, he's evidently busy at the moment.

garex · 2016-11-08T07:25:30Z

@Ocramius As I see, @deeky666 has not any activity on his github profile after 2016, July.

Do you know him? Is he alive/healthy?

Can you assign someone else from 31 people that are in Doctrine's members team?

Ocramius · 2016-11-08T07:52:07Z

No, I cannot. He's pretty much the gatekeeper, and I talk with him from
time to time, he just is that busy IRL ATM.

On 8 Nov 2016 8:25 a.m., "Alexander Ustimenko" notifications@github.com
wrote:

@Ocramius https://github.com/Ocramius As I see, @deeky666
https://github.com/deeky666 has not any activity on his github profile
after 2016, July.

Do you know him? Is he alive/healthy?

Can you assign someone else from 31 people that are in Doctrine's members
team?

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#2490 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAJakPH_vngo2oVNCiUqDOpfF0l53kzRks5q8CPrgaJpZM4JuIOV
.

phansys

Status: Needs work.

phansys · 2016-11-18T14:48:54Z

lib/Doctrine/DBAL/Schema/Visitor/AddDefaultNamespace.php

+ */
+ public function acceptSchema(Schema $schema)
+ {
+ if (!$this->platform->supportsSchemas()) {


Please, respect Doctrine CS.

@phansys which concrete point of them?

I'm guessing it's this one? doctrine/coding-standard#7

Logical NOT operators (!) MUST have leading and trailing spaces

Holy shit, pettifoggers )) I will README https://github.com/doctrine/coding-standard and will come back to you soon.

@teohhanhui it's an opened PR, not yet merged.

I will not change it.

I ran phpcs and it found tons of ERRORs in other files in same directory and nothing in mine. So this is not serious.

$ phpcs --standard=/home/garex/.composer/vendor/doctrine/coding-standard/Doctrine lib/Doctrine/DBAL/Schema/Visitor/* FILE: ...thub/dbal/lib/Doctrine/DBAL/Schema/Visitor/CreateSchemaSqlCollector.php -------------------------------------------------------------------------------- FOUND 3 ERROR(S) AND 1 WARNING(S) AFFECTING 4 LINE(S) -------------------------------------------------------------------------------- 78 | WARNING | Line exceeds 120 characters; contains 127 characters 104 | ERROR | Equals sign not aligned with surrounding assignments; expected | | 4 spaces but found 1 space 105 | ERROR | Equals sign not aligned with surrounding assignments; expected | | 8 spaces but found 1 space 106 | ERROR | Equals sign not aligned with surrounding assignments; expected | | 5 spaces but found 1 space -------------------------------------------------------------------------------- UPGRADE TO PHP_CODESNIFFER 2.0 TO FIX ERRORS AUTOMATICALLY -------------------------------------------------------------------------------- FILE: ...github/dbal/lib/Doctrine/DBAL/Schema/Visitor/DropSchemaSqlCollector.php -------------------------------------------------------------------------------- FOUND 3 ERROR(S) AFFECTING 3 LINE(S) -------------------------------------------------------------------------------- 100 | ERROR | Equals sign not aligned with surrounding assignments; expected 3 | | spaces but found 1 space 101 | ERROR | Equals sign not aligned with surrounding assignments; expected 6 | | spaces but found 1 space 113 | ERROR | Equals sign not aligned with surrounding assignments; expected 6 | | spaces but found 1 space -------------------------------------------------------------------------------- UPGRADE TO PHP_CODESNIFFER 2.0 TO FIX ERRORS AUTOMATICALLY -------------------------------------------------------------------------------- FILE: ...ome/garex/src/github/dbal/lib/Doctrine/DBAL/Schema/Visitor/Graphviz.php -------------------------------------------------------------------------------- FOUND 10 ERROR(S) AND 2 WARNING(S) AFFECTING 6 LINE(S) -------------------------------------------------------------------------------- 42 | ERROR | Missing space before the string concatenation operator ".". 42 | ERROR | Missing space after the string concatenation operator ".". 43 | ERROR | Missing space before the string concatenation operator ".". 43 | ERROR | Missing space after the string concatenation operator ".". 60 | ERROR | Missing space before the string concatenation operator ".". 60 | ERROR | Missing space after the string concatenation operator ".". 90 | WARNING | Line exceeds 120 characters; contains 182 characters 99 | WARNING | Line exceeds 120 characters; contains 181 characters 100 | ERROR | Missing space before the string concatenation operator ".". 100 | ERROR | Missing space after the string concatenation operator ".". 100 | ERROR | Missing space before the string concatenation operator ".". 100 | ERROR | Missing space after the string concatenation operator ".". -------------------------------------------------------------------------------- UPGRADE TO PHP_CODESNIFFER 2.0 TO FIX ERRORS AUTOMATICALLY -------------------------------------------------------------------------------- FILE: ...github/dbal/lib/Doctrine/DBAL/Schema/Visitor/RemoveNamespacedAssets.php -------------------------------------------------------------------------------- FOUND 4 ERROR(S) AFFECTING 4 LINE(S) -------------------------------------------------------------------------------- 61 | ERROR | Expected 0 spaces after opening bracket; 1 found 71 | ERROR | Expected 0 spaces after opening bracket; 1 found 84 | ERROR | Expected 0 spaces after opening bracket; 1 found 90 | ERROR | Expected 0 spaces after opening bracket; 1 found -------------------------------------------------------------------------------- UPGRADE TO PHP_CODESNIFFER 2.0 TO FIX ERRORS AUTOMATICALLY -------------------------------------------------------------------------------- FILE: .../src/github/dbal/lib/Doctrine/DBAL/Schema/Visitor/SchemaDiffVisitor.php -------------------------------------------------------------------------------- FOUND 8 ERROR(S) AFFECTING 8 LINE(S) -------------------------------------------------------------------------------- 41 | ERROR | Visibility must be declared on method "visitOrphanedForeignKey" 48 | ERROR | Visibility must be declared on method "visitChangedSequence" 55 | ERROR | Visibility must be declared on method "visitRemovedSequence" 60 | ERROR | Visibility must be declared on method "visitNewSequence" 65 | ERROR | Visibility must be declared on method "visitNewTable" 71 | ERROR | Visibility must be declared on method "visitNewTableForeignKey" 76 | ERROR | Visibility must be declared on method "visitRemovedTable" 81 | ERROR | Visibility must be declared on method "visitChangedTable" -------------------------------------------------------------------------------- UPGRADE TO PHP_CODESNIFFER 2.0 TO FIX ERRORS AUTOMATICALLY --------------------------------------------------------------------------------

Merge remote-tracking branch 'doctrine/master' into DBAL-1168-add-default-schema-name

deeky666 · 2017-01-14T21:58:57Z

@garex sorry for the delay. Haven't been around here last year very much. The concern I have about the "default schema" topic how it is implemented in DBAL (via platforms) is that there really is no "fixed" default schema for probably all the vendors supporting schemas. In PostgreSQL for example you can configure the default schema via its search path, Oracle uses the user that connects as the default schema and I suppose others like SQL Server let the user define the default schema, too. All that happens at runtime so having the platforms return a default schema name is quite of a misconception IMO and should even be deprecated. Also the term "schema" and "namespace" as used in Doctrine is rather ambiguous and not clearly defined. IMO "schema" / "namespace" is a concept that groups database objects like tables, indexes etc. So in MySQL for example this concept would translate to "database".

So what we have to do (rather than relying on the platforms default schema name) is evaluate the runtime default schema name and add it to the table name when creating the schema form entity classes.

The problem IMO is this line. While assuming that public is the default namespace for all vendors (even those not supporting schemas) is completely wrong, the runtime default name should already be evaluated in the schema manager.

So maybe this needs reevaluation. I don't think this solution is the best we can achieve.

garex · 2017-01-16T09:32:36Z

Hi @deeky666 , nice to see you here.

I think it will be overcomplicated fix that will be merged may be in next century. Current solution is not best, but it will fix issue in 99% of cases.

How often you see users, who change default schema names or default root name? I think it's a rare case.

Anyway if you want more ideal solution for 1% of users, it could be later improved by evaluating from live connection during schema diff. But as I assume it will be low-priority task that will be never done.

deeky666 · 2017-01-17T00:20:47Z

@garex I get your point and you are possibly completely right about that it fixes the issue for "99%" of the users but I think we already have the possibility to fix this more properly without a lot of effort. The schema manager already has an API for evaluating the runtime schema. The reason I don't like the solution is that it pushes a misconception even further into the wrong direction. Also adding a special visitor for this makes us having to maintain that API which in fact IMO is just a workaround that doesn't even fix all edge cases. Speaking of edge cases, there is murphy's law and from my experience the "1%" edge cases will be requested to be fixed soon after that. So introducing this solution will only put us more into the corner.
I could probably live with a solution like this if in fact we do not have another better way of fixing this issue. I am only asking for evaluating if we can use the schema managers getSchemaSearchPaths() for this (because this is exactly the reasons why we implemented it IIRC). And looking at the code it should already do its job. IMO the logic should be:

evaluate the default schema via getSchemaSearchPaths()
if that does not return anything, use the default schema defined in the platform

That would save us an additional visitor, if I am not completely wrong.

garex · 2017-01-17T01:25:03Z

@deeky666 I will research it and ping back here.

antonmedv · 2018-03-27T05:39:01Z

@deeky666 any plans on solving this?

garex · 2018-03-27T05:42:08Z

@antonmedv you can try to fix it yourself. @deeky666 is only as a reviewer here, not implementer/fixer.

greg0ire · 2022-04-07T14:37:58Z

getSchemaSearchPaths() is no longer an option because of #4821 PostgreSQLSchemaManager::getCurrentSchema() should be used instead.

Ocramius reviewed Sep 7, 2016
View reviewed changes

Ocramius added New Feature Schema Management Schema Schema Definition labels Sep 7, 2016

Ocramius reviewed Sep 8, 2016
View reviewed changes

garex added 5 commits September 9, 2016 10:17

!!! TRASH COMMIT NOT FOR MERGE !!!

499ca0d

--- Branch will be rebased after approve --- Merge branch 'master' into DBAL-1168-add-default-schema-name

.. Add docblocks, fix since, wrap text to 80 chars

3fdacc0

.. make visitor noop on incompatible platforms

dafe7ad

.. Test incompatible platforms unaffected

e68433a

.. Fix imports in test

b1e79ef

Ocramius added the Requires Review label Sep 10, 2016

Ocramius assigned deeky666 Sep 10, 2016

phansys suggested changes Sep 26, 2016

View reviewed changes

garex added 2 commits September 26, 2016 12:00

!!! TRASH COMMIT NOT FOR MERGE !!!

ba4fb09

--- Branch will be rebased after approve --- Merge branch 'master' into DBAL-1168-add-default-schema-name

.. Remove surplus LF.

d25aa44

garex mentioned this pull request Oct 17, 2016

Make diff/generate commands more extensible doctrine/migrations#487

Closed

garex mentioned this pull request Oct 25, 2016

DBAL-1168: Schema's getMigrateFromSql always adds CREATE SCHEMA #1110

Closed

phansys suggested changes Nov 18, 2016

View reviewed changes

garex added 2 commits November 22, 2016 14:09

!!! TRASH COMMIT !!! not for future merge

983375d

Merge remote-tracking branch 'doctrine/master' into DBAL-1168-add-default-schema-name

.. phpcs fix in test

7f7b085

vafrcor mentioned this pull request Apr 6, 2017

PostgreSQL\Connection\Params\ How we set Default Search Path for custom-schema namespace? #2697

Closed

morozov removed the Requires Review label Dec 4, 2020

Base automatically changed from master to 4.0.x January 22, 2021 07:43

morozov closed this Oct 26, 2021

github-actions bot locked as resolved and limited conversation to collaborators Apr 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds default namespace to schema. #2490

Adds default namespace to schema. #2490

garex commented Aug 26, 2016 •

edited

Loading

garex commented Aug 26, 2016 •

edited

Loading

Ocramius Sep 7, 2016

garex Sep 8, 2016

Ocramius Sep 8, 2016

garex Sep 8, 2016

teohhanhui Sep 8, 2016

Ocramius commented Sep 7, 2016

garex commented Sep 8, 2016

Ocramius commented Sep 8, 2016

garex commented Sep 8, 2016

Ocramius Sep 8, 2016

garex commented Sep 9, 2016

garex commented Sep 12, 2016

garex commented Sep 23, 2016

phansys Sep 26, 2016

garex Sep 26, 2016

phansys Sep 26, 2016

garex commented Sep 29, 2016

Ocramius commented Sep 29, 2016

garex commented Nov 8, 2016

Ocramius commented Nov 8, 2016

phansys left a comment

phansys Nov 18, 2016

garex Nov 18, 2016

teohhanhui Nov 21, 2016 •

edited

Loading

garex Nov 21, 2016

garex Nov 22, 2016

deeky666 commented Jan 14, 2017

garex commented Jan 16, 2017

deeky666 commented Jan 17, 2017

garex commented Jan 17, 2017

antonmedv commented Mar 27, 2018

garex commented Mar 27, 2018

greg0ire commented Apr 7, 2022 •

edited

Loading

Adds default namespace to schema. #2490

Adds default namespace to schema. #2490

Conversation

garex commented Aug 26, 2016 • edited Loading

garex commented Aug 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ocramius commented Sep 7, 2016

garex commented Sep 8, 2016

Ocramius commented Sep 8, 2016

garex commented Sep 8, 2016

Choose a reason for hiding this comment

garex commented Sep 9, 2016

garex commented Sep 12, 2016

garex commented Sep 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garex commented Sep 29, 2016

Ocramius commented Sep 29, 2016

garex commented Nov 8, 2016

Ocramius commented Nov 8, 2016

phansys left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teohhanhui Nov 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deeky666 commented Jan 14, 2017

garex commented Jan 16, 2017

deeky666 commented Jan 17, 2017

garex commented Jan 17, 2017

antonmedv commented Mar 27, 2018

garex commented Mar 27, 2018

greg0ire commented Apr 7, 2022 • edited Loading

garex commented Aug 26, 2016 •

edited

Loading

garex commented Aug 26, 2016 •

edited

Loading

teohhanhui Nov 21, 2016 •

edited

Loading

greg0ire commented Apr 7, 2022 •

edited

Loading