Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NodeBundle] Revert "Fix bug in NodeChoiceType (#2115)" #2133

Merged
merged 1 commit into from Sep 19, 2018

Conversation

deZinc
Copy link
Contributor

@deZinc deZinc commented Sep 18, 2018

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets

This reverts commit 31d0cbb.

This reverts pull request #2115 because it's current implementation is BC breaking. In the NodeChoiceType, it's possible to overwrite the querybuilder with a custom querybuilder, for example:

        $builder
            ->add('node', NodeChoiceType::class, [
                'required' => true,
                'multiple' => false,
                'attr' => [
                    'class' => 'js-advanced-select',
                ],
                'label' => 'Some label',
                'query_builder' => function (NodeRepository $repo) use ($options) {
                    return $repo->createQueryBuilder('n')
                        ->innerJoin('n.nodeTranslations', 'nt')
                        ->innerJoin('nt.publicNodeVersion', 'nv')
                        ->andWhere(.....
                        ;
                },
            ]) 

With the the version of the NodeChoiceType as in PR #2115 , the page is broken because:

[Semantical Error] line 0, col 259 near 'nt INNER JOIN': Error: 'nt' is already defined.

@wesleylancel Could you have another look at this issue, trying to find a non BC breaking solution?

Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @deZinc, your PR needs some changes

  • It seems that our checklist is missing or incomplete

Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @deZinc, your PR needs some changes

  • This PR seems to need a milestone of a patch release.

Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @deZinc, your PR passed all our requirements.

Thank you for contributing!

@acrobat
Copy link
Member

acrobat commented Sep 18, 2018

Possible fix could be to move the extra checks (like is node online, not deleted etc) to the default query_builder

And change the transformer to only add the refEntityName where if the page_class option is set. This way the default choice type will always work (and will show only online, not deleted nodes) and if someone specified a custom qb, like the example, it won't break.

@Devolicious Devolicious modified the milestones: 5.0.11, 5.0.12 Sep 19, 2018
@Devolicious Devolicious merged commit 86edcd4 into Kunstmaan:5.0 Sep 19, 2018
diskwriter added a commit to diskwriter/KunstmaanBundlesCMS that referenced this pull request Nov 19, 2018
* '5.0' of github.com:Kunstmaan/KunstmaanBundlesCMS:
  [NodeBundle] fix issue finding disctinct classes for deleted files (Kunstmaan#2148)
  Translate pagepart type name in delete modal (Kunstmaan#2160)
  [TranslatorBundle] fix non-unique locales in configurator (Kunstmaan#2156)
  [AdminListBundle] use datepicker for DateTimeType filters (Kunstmaan#2159)
  [Docs] Fix the links in the github contributing.md file (Kunstmaan#2166)
  Remove duplicate set in template (Kunstmaan#2168)
  Fixed issue where a date (and time) in the past could be selected when publishing later (Kunstmaan#2163)
  Improved error shown when a page does not have a view location (Kunstmaan#2162)
  [AdminListBundle] Feature/sortable adminlist (Kunstmaan#2143)
  [AdminBundle] revert ckeditor to original version - manual clone - no npm version available (Kunstmaan#2134)
  [NodeBundle] Revert "Fix bug in NodeChoiceType (Kunstmaan#2115)" (Kunstmaan#2133)
diskwriter pushed a commit to diskwriter/KunstmaanBundlesCMS that referenced this pull request Nov 22, 2018
diskwriter added a commit to diskwriter/KunstmaanBundlesCMS that referenced this pull request Nov 22, 2018
* kunstmaan/5.0:
  [AdminBundle] Fix icons CKeditor (Kunstmaan#2174)
  fix gulp dependency (Kunstmaan#2172)
  null config for older branches (Kunstmaan#2151)
  remove unnecessary roles on dom nodes (Kunstmaan#2170)
  [NodeBundle] fix issue finding disctinct classes for deleted files (Kunstmaan#2148)
  Translate pagepart type name in delete modal (Kunstmaan#2160)
  [TranslatorBundle] fix non-unique locales in configurator (Kunstmaan#2156)
  [AdminListBundle] use datepicker for DateTimeType filters (Kunstmaan#2159)
  [Docs] Fix the links in the github contributing.md file (Kunstmaan#2166)
  Remove duplicate set in template (Kunstmaan#2168)
  Fixed issue where a date (and time) in the past could be selected when publishing later (Kunstmaan#2163)
  Improved error shown when a page does not have a view location (Kunstmaan#2162)
  [AdminListBundle] Feature/sortable adminlist (Kunstmaan#2143)
  [AdminBundle] revert ckeditor to original version - manual clone - no npm version available (Kunstmaan#2134)
  [NodeBundle] Revert "Fix bug in NodeChoiceType (Kunstmaan#2115)" (Kunstmaan#2133)
acrobat added a commit that referenced this pull request Nov 29, 2018
* 5.0:
  Update changelog for 5.0.12 release
  Changes for upcoming travis-ci infra migration (#2180)
  [Docs] Fix slack channel link (#2178)
  [AdminBundle] Fix icons CKeditor (#2174)
  Apply styleci codestyle changes
  Backport styleci fixes to 5.0 branch
  fix gulp dependency (#2172)
  null config for older branches (#2151)
  remove unnecessary roles on dom nodes (#2170)
  [NodeBundle] fix issue finding disctinct classes for deleted files (#2148)
  Translate pagepart type name in delete modal (#2160)
  [TranslatorBundle] fix non-unique locales in configurator (#2156)
  [AdminListBundle] use datepicker for DateTimeType filters (#2159)
  [Docs] Fix the links in the github contributing.md file (#2166)
  Remove duplicate set in template (#2168)
  Fixed issue where a date (and time) in the past could be selected when publishing later (#2163)
  Improved error shown when a page does not have a view location (#2162)
  [AdminListBundle] Feature/sortable adminlist (#2143)
  [AdminBundle] revert ckeditor to original version - manual clone - no npm version available (#2134)
  [NodeBundle] Revert "Fix bug in NodeChoiceType (#2115)" (#2133)

 Conflicts:
	README.md
	TestListener.php
	composer.json
	src/Kunstmaan/AdminBundle/Command/ApplyAclCommand.php
	src/Kunstmaan/AdminBundle/Command/ExceptionCommand.php
	src/Kunstmaan/AdminBundle/Command/UpdateAclCommand.php
	src/Kunstmaan/AdminBundle/DependencyInjection/KunstmaanAdminExtension.php
	src/Kunstmaan/AdminBundle/Entity/AbstractEntity.php
	src/Kunstmaan/AdminBundle/Helper/AdminPanel/DefaultAdminPanelAdaptor.php
	src/Kunstmaan/AdminBundle/Helper/AdminRouteHelper.php
	src/Kunstmaan/AdminBundle/Helper/VersionCheck/VersionChecker.php
	src/Kunstmaan/AdminBundle/KunstmaanAdminBundle.php
	src/Kunstmaan/AdminBundle/Tests/Entity/TestEntity.php
	src/Kunstmaan/AdminBundle/Tests/Form/MediaTokenTransformerTest.php
	src/Kunstmaan/AdminBundle/Tests/Mocks/StatementMock.php
	src/Kunstmaan/AdminBundle/Tests/unit/Entity/AbstractEntityTest.php
	src/Kunstmaan/AdminBundle/Tests/unit/Entity/AclChangesetTest.php
	src/Kunstmaan/AdminBundle/Tests/unit/Entity/GroupTest.php
	src/Kunstmaan/AdminBundle/Tests/unit/Entity/RoleTest.php
	src/Kunstmaan/AdminBundle/Tests/unit/Entity/UserTest.php
	src/Kunstmaan/AdminBundle/Tests/unit/Helper/DomainConfigurationTest.php
	src/Kunstmaan/AdminBundle/Tests/unit/Helper/Menu/MenuItemTest.php
	src/Kunstmaan/AdminBundle/Tests/unit/Helper/Security/Acl/AclHelperTest.php
	src/Kunstmaan/AdminBundle/Tests/unit/Helper/Security/Acl/AclNativeHelperTest.php
	src/Kunstmaan/AdminBundle/Tests/unit/Helper/Security/Acl/Permission/MaskBuilderTest.php
	src/Kunstmaan/AdminBundle/Tests/unit/Helper/Security/Acl/Permission/PermissionAdminTest.php
	src/Kunstmaan/AdminBundle/Tests/unit/Helper/Security/Acl/Permission/PermissionDefinitionTest.php
	src/Kunstmaan/AdminBundle/Tests/unit/Helper/Security/Acl/Permission/PermissionMapTest.php
	src/Kunstmaan/AdminBundle/Twig/MultiDomainAdminTwigExtension.php
	src/Kunstmaan/AdminListBundle/Controller/AdminListController.php
	src/Kunstmaan/AdminListBundle/Tests/AdminList/ExportListTest.php
	src/Kunstmaan/AdminListBundle/Tests/AdminList/FilterType/ORM/ORMFilterTypeTestCase.php
	src/Kunstmaan/AdminListBundle/Tests/AdminList/ItemAction/SimpleItemActionTest.php
	src/Kunstmaan/AdminListBundle/Tests/unit/AdminList/AdminListFactoryTest.php
	src/Kunstmaan/AdminListBundle/Tests/unit/AdminList/FieldTest.php
	src/Kunstmaan/AdminListBundle/Tests/unit/AdminList/FilterBuilderTest.php
	src/Kunstmaan/AdminListBundle/Tests/unit/AdminList/FilterTest.php
	src/Kunstmaan/AdminListBundle/Tests/unit/AdminList/FilterType/DBAL/AbstractDBALFilterTypeTest.php
	src/Kunstmaan/AdminListBundle/Tests/unit/AdminList/FilterType/DBAL/BooleanFilterTypeTest.php
	src/Kunstmaan/AdminListBundle/Tests/unit/AdminList/FilterType/DBAL/DateFilterTypeTest.php
	src/Kunstmaan/AdminListBundle/Tests/unit/AdminList/FilterType/DBAL/DateTimeFilterTypeTest.php
	src/Kunstmaan/AdminListBundle/Tests/unit/AdminList/FilterType/DBAL/EnumerationFilterTypeTest.php
	src/Kunstmaan/AdminListBundle/Tests/unit/AdminList/FilterType/DBAL/NumberFilterTypeTest.php
	src/Kunstmaan/AdminListBundle/Tests/unit/AdminList/FilterType/DBAL/StringFilterTypeTest.php
	src/Kunstmaan/AdminListBundle/Tests/unit/AdminList/FilterType/ORM/AbstractORMFilterTypeTest.php
	src/Kunstmaan/AdminListBundle/Tests/unit/AdminList/FilterType/ORM/BooleanFilterTypeTest.php
	src/Kunstmaan/AdminListBundle/Tests/unit/AdminList/FilterType/ORM/DateFilterTypeTest.php
	src/Kunstmaan/AdminListBundle/Tests/unit/AdminList/FilterType/ORM/DateTimeFilterTypeTest.php
	src/Kunstmaan/AdminListBundle/Tests/unit/AdminList/FilterType/ORM/EnumerationFilterTypeTest.php
	src/Kunstmaan/AdminListBundle/Tests/unit/AdminList/FilterType/ORM/NumberFilterTypeTest.php
	src/Kunstmaan/AdminListBundle/Tests/unit/AdminList/FilterType/ORM/StringFilterTypeTest.php
	src/Kunstmaan/AdminListBundle/Tests/unit/Service/EntityVersionLockServiceTest.php
	src/Kunstmaan/ArticleBundle/Twig/ArticleTwigExtension.php
	src/Kunstmaan/ConfigBundle/Controller/ConfigController.php
	src/Kunstmaan/DashboardBundle/Command/DashboardCommand.php
	src/Kunstmaan/DashboardBundle/Command/GoogleAnalyticsConfigFlushCommand.php
	src/Kunstmaan/DashboardBundle/Command/GoogleAnalyticsDataFlushCommand.php
	src/Kunstmaan/DashboardBundle/Entity/AnalyticsOverview.php
	src/Kunstmaan/FormBundle/Controller/FormSubmissionsController.php
	src/Kunstmaan/FormBundle/Form/ChoiceFormSubmissionType.php
	src/Kunstmaan/FormBundle/Helper/FormMailer.php
	src/Kunstmaan/FormBundle/Tests/AdminList/FormPageAdminListConfiguratorTest.php
	src/Kunstmaan/FormBundle/Tests/Entity/FormSubmissionFieldTypes/FileFormSubmissionFieldTest.php
	src/Kunstmaan/FormBundle/Tests/Stubs/TestConfiguration.php
	src/Kunstmaan/FormBundle/Tests/Stubs/TestRepository.php
	src/Kunstmaan/FormBundle/Tests/unit/Entity/AbstractFormPageTest.php
	src/Kunstmaan/FormBundle/Tests/unit/Entity/FormSubmissionFieldTest.php
	src/Kunstmaan/FormBundle/Tests/unit/Entity/FormSubmissionFieldTypes/ChoiceFormSubmissionFieldTest.php
	src/Kunstmaan/FormBundle/Tests/unit/Entity/FormSubmissionFieldTypes/EmailFormSubmissionFieldTest.php
	src/Kunstmaan/FormBundle/Tests/unit/Entity/FormSubmissionFieldTypes/StringFormSubmissionFieldTest.php
	src/Kunstmaan/FormBundle/Tests/unit/Entity/FormSubmissionFieldTypes/TextFormSubmissionFieldTest.php
	src/Kunstmaan/FormBundle/Tests/unit/Entity/FormSubmissionTest.php
	src/Kunstmaan/FormBundle/Tests/unit/Entity/PageParts/AbstractFormPagePartTest.php
	src/Kunstmaan/FormBundle/Tests/unit/Entity/PageParts/ChoicePagePartTest.php
	src/Kunstmaan/FormBundle/Tests/unit/Entity/PageParts/EmailPagePartTest.php
	src/Kunstmaan/FormBundle/Tests/unit/Entity/PageParts/FileUploadPagePartTest.php
	src/Kunstmaan/FormBundle/Tests/unit/Entity/PageParts/MultiLineTextPagePartTest.php
	src/Kunstmaan/FormBundle/Tests/unit/Entity/PageParts/SingleLineTextPagePartTest.php
	src/Kunstmaan/FormBundle/Tests/unit/Entity/PageParts/SubmitButtonPagePartTest.php
	src/Kunstmaan/GeneratorBundle/Generator/FormPageGenerator.php
	src/Kunstmaan/GeneratorBundle/Resources/SensioGeneratorBundle/skeleton/layout/groundcontrol/package.json
	src/Kunstmaan/GeneratorBundle/Tests/unit/Generator/DefaultSiteGeneratorTest.php
	src/Kunstmaan/GeneratorBundle/Tests/unit/Helper/GeneratorUtilsTest.php
	src/Kunstmaan/MediaBundle/AdminList/MediaAdminListConfigurator.php
	src/Kunstmaan/MediaBundle/Command/CleanDeletedMediaCommand.php
	src/Kunstmaan/MediaBundle/Command/CreatePdfPreviewCommand.php
	src/Kunstmaan/MediaBundle/Command/RenameSoftDeletedCommand.php
	src/Kunstmaan/MediaBundle/Controller/MediaController.php
	src/Kunstmaan/MediaBundle/Form/FolderType.php
	src/Kunstmaan/MediaBundle/Tests/unit/Entity/FolderTest.php
	src/Kunstmaan/MediaBundle/Tests/unit/Entity/MediaTest.php
	src/Kunstmaan/MediaBundle/Tests/unit/Helper/File/FileHelperTest.php
	src/Kunstmaan/MediaBundle/Tests/unit/Helper/File/PdfHandlerTest.php
	src/Kunstmaan/MediaBundle/Tests/unit/Helper/FolderManagerTest.php
	src/Kunstmaan/MediaBundle/Tests/unit/Helper/MediaManagerTest.php
	src/Kunstmaan/MediaBundle/Tests/unit/Helper/RemoteAudio/RemoteAudioHelperTest.php
	src/Kunstmaan/MediaBundle/Tests/unit/Helper/RemoteSlide/RemoteSlideHelperTest.php
	src/Kunstmaan/MediaBundle/Tests/unit/Helper/RemoteVideo/RemoteVideoHelperTest.php
	src/Kunstmaan/MediaBundle/Tests/unit/Helper/Transformer/PdfTransformerTest.php
	src/Kunstmaan/MediaPagePartBundle/Tests/Controller/DefaultControllerTest.php
	src/Kunstmaan/MediaPagePartBundle/Tests/unit/Entity/DownloadPagePartTest.php
	src/Kunstmaan/MediaPagePartBundle/Tests/unit/Entity/ImagePagePartTest.php
	src/Kunstmaan/MediaPagePartBundle/Tests/unit/Entity/SlidePagePartTest.php
	src/Kunstmaan/MediaPagePartBundle/Tests/unit/Entity/VideoPagePartTest.php
	src/Kunstmaan/MultiDomainBundle/Tests/Router/DomainBasedLocaleRouterTest.php
	src/Kunstmaan/MultiDomainBundle/Tests/unit/Helper/AdminPanel/SitesAdminPanelAdaptorTest.php
	src/Kunstmaan/MultiDomainBundle/Tests/unit/Helper/DomainConfigurationTest.php
	src/Kunstmaan/MultiDomainBundle/Tests/unit/Helper/HostOverrideCleanupHandlerTest.php
	src/Kunstmaan/NodeBundle/Command/CronUpdateNodeCommand.php
	src/Kunstmaan/NodeBundle/Command/InitAclCommand.php
	src/Kunstmaan/NodeBundle/EventListener/RenderContextListener.php
	src/Kunstmaan/NodeBundle/Form/EventListener/URLChooserFormSubscriber.php
	src/Kunstmaan/NodeBundle/Helper/Menu/ActionsMenuBuilder.php
	src/Kunstmaan/NodeBundle/Helper/Menu/PageMenuAdaptor.php
	src/Kunstmaan/NodeBundle/Helper/PagesConfiguration.php
	src/Kunstmaan/NodeBundle/KunstmaanNodeBundle.php
	src/Kunstmaan/NodeBundle/Router/SlugRouter.php
	src/Kunstmaan/NodeBundle/Tests/Entity/TestEntity.php
	src/Kunstmaan/NodeBundle/Tests/Stubs/TestRepository.php
	src/Kunstmaan/NodeBundle/Tests/unit/Entity/NodeTest.php
	src/Kunstmaan/NodeBundle/Tests/unit/Entity/NodeTranslationTest.php
	src/Kunstmaan/NodeBundle/Tests/unit/Entity/NodeVersionTest.php
	src/Kunstmaan/NodeBundle/Tests/unit/Helper/Menu/ActionsMenuBuilderTest.php
	src/Kunstmaan/NodeBundle/Tests/unit/Router/SlugRouterTest.php
	src/Kunstmaan/NodeSearchBundle/DependencyInjection/KunstmaanNodeSearchExtension.php
	src/Kunstmaan/PagePartBundle/Tests/Form/TextPagePartAdminTypeTest.php
	src/Kunstmaan/PagePartBundle/Tests/unit/Entity/HeaderPagePartTest.php
	src/Kunstmaan/PagePartBundle/Tests/unit/Entity/LinePagePartTest.php
	src/Kunstmaan/PagePartBundle/Tests/unit/Entity/LinkPagePartTest.php
	src/Kunstmaan/PagePartBundle/Tests/unit/Entity/RawHTMLPagePartTest.php
	src/Kunstmaan/PagePartBundle/Tests/unit/Entity/TextPagePartTest.php
	src/Kunstmaan/PagePartBundle/Tests/unit/Entity/ToTopPagePartTest.php
	src/Kunstmaan/PagePartBundle/Tests/unit/Entity/TocPagePartTest.php
	src/Kunstmaan/PagePartBundle/Tests/unit/EventListener/CloneListenerTest.php
	src/Kunstmaan/PagePartBundle/Tests/unit/Form/HeaderPagePartAdminTypeTest.php
	src/Kunstmaan/PagePartBundle/Tests/unit/Form/LinePagePartAdminTypeTest.php
	src/Kunstmaan/PagePartBundle/Tests/unit/Form/LinkPagePartAdminTypeTest.php
	src/Kunstmaan/PagePartBundle/Tests/unit/Form/RawHTMLPagePartAdminTypeTest.php
	src/Kunstmaan/PagePartBundle/Tests/unit/Form/ToTopPagePartAdminTypeTest.php
	src/Kunstmaan/PagePartBundle/Tests/unit/Form/TocPagePartAdminTypeTest.php
	src/Kunstmaan/RedirectBundle/Tests/Entity/RedirectTest.php
	src/Kunstmaan/RedirectBundle/Tests/unit/AdminList/RedirectAdminListConfiguratorTest.php
	src/Kunstmaan/RedirectBundle/Tests/unit/Form/RedirectAdminTypeTest.php
	src/Kunstmaan/RedirectBundle/Tests/unit/Router/RedirectRouterTest.php
	src/Kunstmaan/SearchBundle/Command/SetupIndexCommand.php
	src/Kunstmaan/SearchBundle/Tests/unit/DependencyInjection/Configuration/SearchConfigurationChainTest.php
	src/Kunstmaan/SeoBundle/Tests/unit/Entity/SeoTest.php
	src/Kunstmaan/SeoBundle/Tests/unit/Twig/TwigExtensionTests.php
	src/Kunstmaan/TaggingBundle/Entity/TagManager.php
	src/Kunstmaan/TranslatorBundle/Command/ExportTranslationsCommand.php
	src/Kunstmaan/TranslatorBundle/Command/ImportTranslationsCommand.php
	src/Kunstmaan/TranslatorBundle/Command/TranslationCacheCommand.php
	src/Kunstmaan/TranslatorBundle/Controller/TranslatorController.php
	src/Kunstmaan/TranslatorBundle/Entity/Translation.php
	src/Kunstmaan/TranslatorBundle/Service/Command/Importer/ImportCommandHandler.php
	src/Kunstmaan/TranslatorBundle/Service/Command/Importer/Importer.php
	src/Kunstmaan/TranslatorBundle/Service/Translator/Translator.php
	src/Kunstmaan/TranslatorBundle/Tests/BaseTestCase.php
	src/Kunstmaan/TranslatorBundle/Tests/TestListener.php
	src/Kunstmaan/TranslatorBundle/Tests/unit/DependencyInjection/KunstmaanTranslatorExtensionTest.php
	src/Kunstmaan/TranslatorBundle/Tests/unit/Repository/TranslationRepositoryTest.php
	src/Kunstmaan/TranslatorBundle/Toolbar/TranslatorDataCollector.php
	src/Kunstmaan/UserManagementBundle/Controller/GroupsController.php
	src/Kunstmaan/UserManagementBundle/Controller/RolesController.php
	src/Kunstmaan/UserManagementBundle/Controller/UsersController.php
	src/Kunstmaan/UtilitiesBundle/Helper/Shell/Shell.php
	src/Kunstmaan/UtilitiesBundle/Tests/Helper/ClassLookupTest.php
	src/Kunstmaan/UtilitiesBundle/Tests/Helper/DummyClass.php
	src/Kunstmaan/UtilitiesBundle/Tests/unit/Helper/Cipher/CipherTest.php
	src/Kunstmaan/VotingBundle/Tests/Services/RepositoryResolverTest.php
	src/Kunstmaan/VotingBundle/Tests/unit/DependencyInjection/KunstmaanVotingExtensionTest.php
	src/Kunstmaan/VotingBundle/Tests/unit/EventListener/Security/MaxNumberByIpEventListenerTest.php
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants