Unable to delete blocks: RESTRICT on parent_id foreign key. What (offending commit inside)?

gremo commented 4 years ago

Environment

Sonata packages

$ composer show --latest 'sonata-project/*'
sonata-project/admin-bundle              3.69.0 3.69.0 The missing Symfony Admin Generator
sonata-project/block-bundle              3.19.0 4.2.0  Symfony SonataBlockBundle
sonata-project/cache                     1.1.1  2.0.1  Cache library
sonata-project/cache-bundle              2.4.2  3.2.1  This bundle provides caching services
sonata-project/classification-bundle     3.11.1 3.11.1 Symfony SonataClassificationBundle
sonata-project/core-bundle               3.20.0 3.20.0 Symfony SonataCoreBundle (abandoned)
sonata-project/datagrid-bundle           2.5.0  3.2.0  Symfony SonataDatagridBundle
sonata-project/doctrine-extensions       1.6.0  1.6.0  Doctrine2 behavioral extensions
sonata-project/doctrine-orm-admin-bundle 3.18.0 3.18.0 Symfony Sonata / Integrate Doctrine ORM into the SonataAdminBundle
sonata-project/easy-extends-bundle       2.5.0  2.5.0  Symfony SonataEasyExtendsBundle
sonata-project/exporter                  2.2.0  2.2.0  Lightweight Exporter library
sonata-project/form-extensions           0.1.2  1.4.0  Symfony form extensions
sonata-project/formatter-bundle          4.1.3  4.1.3  Symfony SonataFormatterBundle
sonata-project/intl-bundle               2.7.0  2.7.0  Symfony SonataIntlBundle
sonata-project/media-bundle              3.24.0 3.24.0 Symfony SonataMediaBundle
sonata-project/notification-bundle       3.7.0  3.7.0  Symfony SonataNotificationBundle
sonata-project/page-bundle               3.17.2 3.17.2 This bundle provides a Site and Page management through container and block services
sonata-project/seo-bundle                2.10.0 2.10.0 Symfony SonataSeoBundle
sonata-project/twig-extensions           0.1.1  1.3.0  Sonata twig extensions
sonata-project/user-bundle               4.5.3  4.5.3  Symfony SonataUserBundle

Symfony packages

$ composer show --latest 'symfony/*'
symfony/apache-pack                v1.0.1  v1.0.1  A pack for Apache support in Symfony
symfony/asset                      v4.4.10 v4.4.10 Symfony Asset Component
symfony/browser-kit                v4.4.10 v4.4.10 Symfony BrowserKit Component
symfony/cache                      v4.4.10 v4.4.10 Symfony Cache component with PSR-6, PSR-16, and tags
symfony/cache-contracts            v2.1.2  v2.1.2  Generic abstractions related to caching
symfony/config                     v4.4.10 v4.4.10 Symfony Config Component
symfony/console                    v4.4.10 v4.4.10 Symfony Console Component
symfony/css-selector               v4.4.10 v4.4.10 Symfony CssSelector Component
symfony/debug                      v4.4.10 v4.4.10 Symfony Debug Component
symfony/debug-bundle               v4.4.10 v4.4.10 Symfony DebugBundle
symfony/debug-pack                 v1.0.8  v1.0.8  A debug pack for Symfony projects
symfony/dependency-injection       v4.4.10 v4.4.10 Symfony DependencyInjection Component
symfony/deprecation-contracts      v2.1.2  v2.1.2  A generic function and convention to trigger deprecation notices
symfony/doctrine-bridge            v4.4.10 v4.4.10 Symfony Doctrine Bridge
symfony/dom-crawler                v4.4.10 v4.4.10 Symfony DomCrawler Component
symfony/dotenv                     v4.4.10 v4.4.10 Registers environment variables from a .env file
symfony/error-handler              v4.4.10 v4.4.10 Symfony ErrorHandler Component
symfony/event-dispatcher           v4.4.10 v4.4.10 Symfony EventDispatcher Component
symfony/event-dispatcher-contracts v1.1.7  v2.1.2  Generic abstractions related to dispatching event
symfony/expression-language        v4.4.10 v4.4.10 Symfony ExpressionLanguage Component
symfony/filesystem                 v4.4.10 v4.4.10 Symfony Filesystem Component
symfony/finder                     v4.4.10 v4.4.10 Symfony Finder Component
symfony/flex                       v1.8.1  v1.8.1  Composer plugin for Symfony
symfony/form                       v4.4.10 v4.4.10 Symfony Form Component
symfony/framework-bundle           v4.4.10 v4.4.10 Symfony FrameworkBundle
symfony/http-client                v4.4.10 v4.4.10 Symfony HttpClient component
symfony/http-client-contracts      v2.1.2  v2.1.2  Generic abstractions related to HTTP clients
symfony/http-foundation            v4.4.10 v4.4.10 Symfony HttpFoundation Component
symfony/http-kernel                v4.4.10 v4.4.10 Symfony HttpKernel Component
symfony/inflector                  v4.4.10 v4.4.10 Symfony Inflector Component
symfony/intl                       v4.4.10 v4.4.10 A PHP replacement layer for the C intl extension that includes additional data from the ICU library.
symfony/mailer                     v4.4.10 v4.4.10 Symfony Mailer Component
symfony/maker-bundle               v1.19.0 v1.19.0 Symfony Maker helps you create empty commands, controllers, form classes, tests and more so you can forget ab...  
symfony/mime                       v4.4.10 v4.4.10 A library to manipulate MIME messages
symfony/monolog-bridge             v4.4.10 v4.4.10 Symfony Monolog Bridge
symfony/monolog-bundle             v3.5.0  v3.5.0  Symfony MonologBundle
symfony/options-resolver           v4.4.10 v4.4.10 Symfony OptionsResolver Component
symfony/orm-pack                   v1.0.8  v1.0.8  A pack for the Doctrine ORM
symfony/phpunit-bridge             v5.1.1  v5.1.1  Symfony PHPUnit Bridge
symfony/polyfill-intl-grapheme     v1.17.0 v1.17.0 Symfony polyfill for intl's grapheme_* functions
symfony/polyfill-intl-icu          v1.17.0 v1.17.0 Symfony polyfill for intl's ICU-related data and classes
symfony/polyfill-intl-idn          v1.17.0 v1.17.0 Symfony polyfill for intl's idn_to_ascii and idn_to_utf8 functions
symfony/polyfill-intl-normalizer   v1.17.0 v1.17.0 Symfony polyfill for intl's Normalizer class and related functions
symfony/polyfill-mbstring          v1.17.0 v1.17.0 Symfony polyfill for the Mbstring extension
symfony/polyfill-php72             v1.17.0 v1.17.0 Symfony polyfill backporting some PHP 7.2+ features to lower PHP versions
symfony/polyfill-php73             v1.17.0 v1.17.0 Symfony polyfill backporting some PHP 7.3+ features to lower PHP versions
symfony/polyfill-php80             v1.17.0 v1.17.0 Symfony polyfill backporting some PHP 8.0+ features to lower PHP versions
symfony/process                    v4.4.10 v4.4.10 Symfony Process Component
symfony/profiler-pack              v1.0.4  v1.0.4  A pack for the Symfony web profiler
symfony/property-access            v4.4.10 v4.4.10 Symfony PropertyAccess Component
symfony/property-info              v4.4.10 v4.4.10 Symfony Property Info Component
symfony/routing                    v4.4.10 v4.4.10 Symfony Routing Component
symfony/security-acl               v3.0.4  v3.0.4  Symfony Security Component - ACL (Access Control List)
symfony/security-bundle            v4.4.10 v4.4.10 Symfony SecurityBundle
symfony/security-core              v4.4.10 v4.4.10 Symfony Security Component - Core Library
symfony/security-csrf              v4.4.10 v4.4.10 Symfony Security Component - CSRF Library
symfony/security-guard             v4.4.10 v4.4.10 Symfony Security Component - Guard
symfony/security-http              v4.4.10 v4.4.10 Symfony Security Component - HTTP Integration
symfony/serializer                 v4.4.10 v4.4.10 Symfony Serializer Component
symfony/serializer-pack            v1.0.3  v1.0.3  A pack for the Symfony serializer
symfony/service-contracts          v2.1.2  v2.1.2  Generic abstractions related to writing services
symfony/stopwatch                  v4.4.10 v4.4.10 Symfony Stopwatch Component
symfony/string                     v5.1.1  v5.1.1  Symfony String component
symfony/swiftmailer-bundle         v3.4.0  v3.4.0  Symfony SwiftmailerBundle
symfony/templating                 v4.4.10 v4.4.10 Symfony Templating Component
symfony/test-pack                  v1.0.6  v1.0.6  A pack for functional and end-to-end testing within a Symfony app
symfony/translation                v4.4.10 v4.4.10 Symfony Translation Component
symfony/translation-contracts      v2.1.2  v2.1.2  Generic abstractions related to translation
symfony/twig-bridge                v4.4.10 v4.4.10 Symfony Twig Bridge
symfony/twig-bundle                v4.4.10 v4.4.10 Symfony TwigBundle
symfony/twig-pack                  v1.0.0  v1.0.0  A Twig pack for Symfony projects
symfony/validator                  v4.4.10 v4.4.10 Symfony Validator Component
symfony/var-dumper                 v4.4.10 v4.4.10 Symfony mechanism for exploring and dumping PHP variables
symfony/var-exporter               v4.4.10 v4.4.10 A blend of var_export() + serialize() to turn any serializable data structure to plain PHP code
symfony/web-profiler-bundle        v4.4.10 v4.4.10 Symfony WebProfilerBundle
symfony/web-server-bundle          v4.4.10 v4.4.10 Symfony WebServerBundle
symfony/webpack-encore-bundle      v1.7.3  v1.7.3  Integration with your Symfony app & Webpack Encore!
symfony/yaml                       v4.4.10 v4.4.10 Symfony Yaml Component

PHP version

$ php -v
PHP 7.4.5 (cli) (built: Apr 14 2020 16:17:19) ( NTS Visual C++ 2017 x64 )
Copyright (c) The PHP Group
Zend Engine v3.4.0, Copyright (c) Zend Technologies
    with Zend OPcache v7.4.5, Copyright (c), by Zend Technologies

Subject

The CASCADEoption for the foreign key parent_id has been removed from the . Defaults to RESTRICT. This is the offending commit.

Steps to reproduce

Create a block using the page composer. Assign it to a "zone" defined using the configuration. Then remove it.

Cattura

Expected results

The block is removed.

Actual results

Throws, because the database fails to remove the row.

gremo commented 4 years ago

Just to be clear: @wbloszyk you committed this with @OskarStark . @greg0ire you released this in 3.17.1.

Commit 331446b was included in release 3.17.1 changing the foreign keys behavior.

Is a patch version (from 3.17.0 to 3.17.1) supposed to do such a breaking change? Does commits gets reviewed, in terms of semantic version and impact on other people applications/sites? Why when something is updated in the Sonata ecosystem... is guaranteed that something will break? Why Sonata seems to be the least stable software in the PHP world?

VincentLanglet commented 4 years ago

Is a patch version (from 3.17.0 to 3.17.1) supposed to do such a breaking change?

No

Does commits gets reviewed, in terms of semantic version and impact on other people applications/sites?

We try to.

Why when something is updated in the Sonata ecosystem... is guaranteed that something will break? Why Sonata seems to be the least stable software in the PHP world?

This can happen with open source. There is a lot of work to do and only a few contributors, working on they spare time for Sonata with no sponsor. I you're no happy with it and think you can do better, build your own software ; nobody force you to use it ;)

greg0ire commented 4 years ago

Why Sonata seems to be the least stable software in the PHP world?

Because you @gremo do not review the PRs. Please watch the repositories and start doing so. If that does not work out, at least you will have gained a deep understanding of why such failures can happen.

wbloszyk commented 4 years ago

@gremo

Just to be clear: @wbloszyk you committed this with @OskarStark . @greg0ire you released this in 3.17.1.

Agree

Commit 331446b was included in release 3.17.1 changing the foreign keys behavior.

Agree

Is a patch version (from 3.17.0 to 3.17.1) supposed to do such a breaking change? Does commits gets reviewed, in terms of semantic version and impact on other people applications/sites? Why when something is updated in the Sonata ecosystem... is guaranteed that something will break? Why Sonata seems to be the least stable software in the PHP world?

Are you read LICENSE? Are you read PR descriptions? Are you revert this PR and check is it working?

gremo commented 4 years ago

Is a patch version (from 3.17.0 to 3.17.1) supposed to do such a breaking change?

No

Does commits gets reviewed, in terms of semantic version and impact on other people applications/sites?

We try to.

Why when something is updated in the Sonata ecosystem... is guaranteed that something will break? Why Sonata seems to be the least stable software in the PHP world?

This can happen with open source. There is a lot of work to do and only a few contributors, working on they spare time for Sonata with no sponsor. I you're no happy with it and think you can do better, build your own software ; nobody force you to use it ;)

Just because it's open source it has not to be bad, full of bugs or not following semver, at least. And for sure, can't put that commit in a patch version.

gremo commented 4 years ago

@gremo

Just to be clear: @wbloszyk you committed this with @OskarStark . @greg0ire you released this in 3.17.1.

Agree

Commit 331446b was included in release 3.17.1 changing the foreign keys behavior.

Agree

Is a patch version (from 3.17.0 to 3.17.1) supposed to do such a breaking change? Does commits gets reviewed, in terms of semantic version and impact on other people applications/sites? Why when something is updated in the Sonata ecosystem... is guaranteed that something will break? Why Sonata seems to be the least stable software in the PHP world?

Are you read LICENSE? Are you read PR descriptions? Are you revert this PR and check is it working?

Honestly, can't understand what you are saying. It's your PR, you fixed some related to MSSQL, good to know.

Your PR says a generic sentence: change some database cascade delete. So, how can people migrate to a new settings? Where is documented?

gremo commented 4 years ago

Why Sonata seems to be the least stable software in the PHP world?

Because you @gremo do not review the PRs. Please watch the repositories and start doing so. If that does not work out, at least you will have gained a deep understanding of why such failures can happen.

This can't be a serious answer. Instead of explaining why you actually published 3.17.1 with a big BC, you prefer to make some irony and get some thumbs up.

You know (or can't you remember?) that I contribute to Sonata project with some, probably minor, fixes and patches. You can always ping if you like to review some PR, if you are too busy with your work.

greg0ire commented 4 years ago

This can't be a serious answer

It is not completely serious as you guessed. If you want an explanation, I just didn't spot the BC break. Are you sure you would have spotted it if you were in my shoes?

You know (or can't you remember?) that I contribute to Sonata project with some, probably minor, fixes and patches.

I know full well and sorry, but it doesn't warrant such a behavior. As thankful as we are for your contributions, it doesn't mean we can accept your bullying.

You can always ping if you like to review some PR, if you are too busy with your work.

The influx of PRs is constant, if I start pinging you on PRs I don't have time to review, you are going to go literally crazy. That's where my answer is not completely joking. If you really want to know why there can be this kind of mistakes, just try keeping up with the PRs on Sonata for a while, you will inevitably end up missing something because it's just too much. Maybe after that you will be more compassionate to people that literally do free work for you.

gremo commented 4 years ago

@greg0ire don't get me wrong. I'm to trying to make you feel "guilty" or blame @wbloszyk for a totally unintelligible PR. I also love open source, made a few projects, trying to help people and trying to not break other people work. Open source doesn't mean "I don't give a f*** if something breaks, we do what we want, read the LICENSE". There should be a minimum of responsibility.

I'm not bullying just because I've submitted a few PR. I'm just saying that Sonata could be a good piece of software, but we/you MUST pay attention when people submit such a "delicate" PR and not blindly accept, merge and release such undocumented change.

Anyways, back to the topic, I think that:

you should completely revert that PR, avoiding breaking other people work; this seems to be urgent
think about this "MSSQL compatibility". Is really necessary to remove the CASCADE behavior? Can we detect the platform and selectively change this behavior? If not, well document such a breaking change.

wbloszyk commented 4 years ago

Let's leave this discussion. We have issue to resolve.

For now sonata-project don't support all sql platform (mssql, probably some other too). https://github.com/sonata-project/dev-kit/issues/689

I think all sonata-project working on doctrine, so we can extends bundles configuration by new option entity_manager : '@doctrine.entity_manager'. From this manager we can get platform. Then we can load cascade behaviors for mysql, triggers for mssql ...

gremo commented 4 years ago

@wbloszyk can you explain what exactly doesn't work with MSSQL? If I understand right, Doctrine can't support MSSQL?

Database cascade is not supported (that's why you removed the line 'onDelete' => 'CASCADE')
Doctrine cascade behaviour is not supported (that's why you removed the line remove)

greg0ire commented 4 years ago

such undocumented change

It's documented at length here: https://github.com/sonata-project/dev-kit/issues/689 (which is linked in the PR, BTW)

@greg0ire don't get me wrong. I'm to trying to make you feel "guilty" or blame @wbloszyk for a totally unintelligible PR.

Your should proofread your sentence, one could argue this one is unintelligible if they wanted to be sarcastic. And with your tone, I can tell that this feel a bit judgmental, coming from someone who maintains OSS projects that are nowhere near the size of Sonata. What you achieved looks great, but sorry, it's just not the same thing. You don't get several pages of Github notifications per day, do you?

That said, I agree with you that this should be reverted, you (or anybody really, but certainly not me) can make a revert PR, and I will merge and release it, then we can make another attempt to get actual compatibility with all DBs, and give the foreign key issue a closer look.

gremo commented 4 years ago

@greg0ire ok let' stop. I agree with you that the number of Sonata issues is really high in respect of a few project I maintain. But there is a community, just ask if you need some help. Sorry for being too much aggressive.

Anyways, I recently made a project using (as second doctrine connection) a MSSQL database. I never had issues at least with onDelete="CASCADE" issue. Didn't tested any cascade Doctrine behavior yet.

I can post some dumps if you ask or any other information.

Example property:

/**
 * @ORM\ManyToOne(targetEntity="RmaRequest", inversedBy="lines")
 * @ORM\JoinColumn(nullable=false, onDelete="CASCADE")
 *
 * @var null|RmaRequest
 */
private $rmaRequest;

I was using ODBC driver for Linux, version 13. No warnings or errors, it just work as expected.

EDIT: now I see that this is related to cycles (that i don't have).

greg0ire commented 4 years ago

Sorry for being too much aggressive.

Apology accepted. I understand how frustrated you are. To answer your questions more fully, I don't use Sonata, which means I don't even have a project on which to test things. Besides, I wouldn't know where to start when it comes to installing mssql. And last but not least, it would just take too much time, which means we rely solely on unit tests and static analysis for catching bugs. IIRC there have been attemps to add functional tests, but I don't think they ended up being merged. I will let you sort this out with @wbloszyk, you're in a much better position than me to fix this.

wbloszyk commented 4 years ago

@gremo @greg0ire

All (expect 1) my contributions are for sonata-project. As you wrote sonata-project is huge OSS project. I using it in almost all my projects. This mean it is very important for me to keep it bug free. That why i working on sandbox 3.0 where we can run functional test based on behat.

My commits make this bug. I made it base on: https://stackoverflow.com/questions/27472538/cascade-remove-vs-orphanremoval-true-vs-ondelete-cascade

I will check it again and try release fix today.

sonata-project / SonataPageBundle