Uploaded image for project: 'eZ Publish / Platform'
  1. eZ Publish / Platform
  2. EZP-23589

Preview issue when relying on locations for unpublished content

    Details

      Description

      There appears to be a regression in the eZ Publish 5.2 version of the patch from issue https://jira.ez.no/browse/EZP-22781, that occurs in the conditions described below.

      Steps to reproduce:

      1. Prepare an eZ Publish 5.2 installation, fully patched;
      2. Install eZ Publish 5.2 patch from https://jira.ez.no/browse/EZP-22781;
      3. Preview the first draft of a page that is being served by a new stack full view template with an Image attribute. If the page is published preview doesn't crash. The problem appears to be happening if you have a custom controller that is trying to access the location ID. In the example below, it is the line that is trying loadLocation() that is failing:

      public function webinarEnterpriseAction( $locationId, $viewType, $layout = false, array $params = array() ) {
      	$params += array();
       
      	$repository = $this->getRepository();
      	$location = $repository->getLocationService()->loadLocation( $locationId );
      	$content = $repository->getContentService()->loadContentByContentInfo( $location->contentInfo );
       
      	$params['tz_dates'] = $this->globalWebinarDates( $content );
       
      	// Forward the request to the Default Controller
      	$response = $this->get( 'ez_content' )->viewLocation( $locationId, $viewType, $layout, $params );
       
      	return $response;
      }
      

      In the customer's case, it happens that their site has an Image attribute on that content class. Therefore, to test you just have to create a custom controller and try to access the location ID. It will always crash on a first draft:

       
       
      Full stack trace is below. Is there a follow-up patch that needs to be applied as well?
       
      [1] eZPublishCoreBaseExceptionsNotFoundException: Could not find 'location' with identifier
      at n/a
      in /var/www/vendor/ezsystems/ezpublish-kernel/eZ/Publish/Core/Persistence/Legacy/Content/Location/Gateway/EzcDatabase.php line 79
       
      at eZPublishCorePersistenceLegacyContentLocationGatewayEzcDatabase->getBasicNodeData(null)
      in /var/www/vendor/ezsystems/ezpublish-kernel/eZ/Publish/Core/Persistence/Legacy/Content/Location/Gateway/ExceptionConversion.php line 56
       
      at eZPublishCorePersistenceLegacyContentLocationGatewayExceptionConversion->getBasicNodeData(null)
      in /var/www/vendor/ezsystems/ezpublish-kernel/eZ/Publish/Core/Persistence/Legacy/Content/Location/Handler.php line 100
       
      at eZPublishCorePersistenceLegacyContentLocationHandler->load(null)
      in /var/www/vendor/ezsystems/ezpublish-kernel/eZ/Publish/Core/Persistence/Cache/LocationHandler.php line 32
       
      at eZPublishCorePersistenceCacheLocationHandler->load(null)
      in /var/www/vendor/ezsystems/ezpublish-kernel/eZ/Publish/Core/Repository/LocationService.php line 205
       
      at eZPublishCoreRepositoryLocationService->loadLocation(null)
      in /var/www/vendor/ezsystems/ezpublish-kernel/eZ/Publish/Core/SignalSlot/LocationService.php line 102
       
      at eZPublishCoreSignalSlotLocationService->loadLocation(null)
      in /var/www/src/Hootsuite/BaseBundle/Controller/WebinarsController.php line 925
       
      at HootsuiteBaseBundleControllerWebinarsController->webinarEnterpriseAction(null, 'full', true, array('content' => object(Content), 'location' => object(Location), 'isPreview' => true))
      in line
       
      at call_user_func_array(array(object(WebinarsController), 'webinarEnterpriseAction'), array(null, 'full', true, array('content' => object(Content), 'location' => object(Location), 'isPreview' => true)))
      in /var/www/ezpublish/bootstrap.php.cache line 2889
       
      at SymfonyComponentHttpKernelHttpKernel->handleRaw(object(Request), '2')
      in /var/www/ezpublish/bootstrap.php.cache line 2863
       
      at SymfonyComponentHttpKernelHttpKernel->handle(object(Request), '2', true)
      in /var/www/ezpublish/bootstrap.php.cache line 2992
       
      at SymfonyComponentHttpKernelDependencyInjectionContainerAwareHttpKernel->handle(object(Request), '2')
      in /var/www/vendor/ezsystems/ezpublish-kernel/eZ/Publish/Core/MVC/Symfony/Controller/Content/PreviewController.php line 89
       
      at eZPublishCoreMVCSymfonyControllerContentPreviewController->previewContentAction('3461', '1', 'eng-US', 'eng')
      in line
       
      at call_user_func_array(array(object(PreviewController), 'previewContentAction'), array('3461', '1', 'eng-US', 'eng'))
      in /var/www/ezpublish/bootstrap.php.cache line 2889
       
      at SymfonyComponentHttpKernelHttpKernel->handleRaw(object(Request), '1')
      in /var/www/ezpublish/bootstrap.php.cache line 2863
       
      at SymfonyComponentHttpKernelHttpKernel->handle(object(Request), '1', true)
      in /var/www/ezpublish/bootstrap.php.cache line 2992
       
      at SymfonyComponentHttpKernelDependencyInjectionContainerAwareHttpKernel->handle(object(Request), '1', true)
      in /var/www/ezpublish/bootstrap.php.cache line 2272
       
      at SymfonyComponentHttpKernelKernel->handle(object(Request), '1', true)
      in /var/www/vendor/ezsystems/ezpublish-kernel/eZ/Bundle/EzPublishCoreBundle/Kernel.php line 62
       
      at eZBundleEzPublishCoreBundleKernel->handle(object(Request))
      in /var/www/web/index.php line 64
      

        Issue Links

          Activity

          Nuno Oliveira (Inactive) created issue -
          Nuno Oliveira (Inactive) made changes -
          Field Original Value New Value
          Summary Preview issue after installing patch from https://jira.ez.no/browse/EZP-22781 (possible regression) Preview issue after installing patch from EZP-22781 (possible regression)
          Nuno Oliveira (Inactive) made changes -
          Description There appears to be a regression in the eZ Publish 5.2 version of the patch from issue https://jira.ez.no/browse/EZP-22781, that occurs in the conditions described below.

          h4. Steps to reproduce:

          1. Prepare an eZ Publish 5.2 installation, fully patched;
          2. Install eZ Publish 5.2 patch from https://jira.ez.no/browse/EZP-22781;
          3. Preview *the first draft* of a page that is being served by a new stack full view template with an Image attribute. If the page is published preview doesn't crash. The problem appears to be

          happening if you have a custom controller that is trying to access the location ID. In the example below, it is the line that is trying loadLocation() that is failing:

          {noformat}
          public function webinarEnterpriseAction( $locationId, $viewType, $layout = false, array $params = array() ) {
          $params += array();

          $repository = $this->getRepository();
          $location = $repository->getLocationService()->loadLocation( $locationId );
          $content = $repository->getContentService()->loadContentByContentInfo( $location->contentInfo );

          $params['tz_dates'] = $this->globalWebinarDates( $content );

          // Forward the request to the Default Controller
          $response = $this->get( 'ez_content' )->viewLocation( $locationId, $viewType, $layout, $params );

          return $response;
          }
          {noformat}

          In the customer's case, it happens that their site has an Image attribute on that content class. Therefore, to test you just have to create a custom controller and try to access the location ID. It will always crash on a first draft:

          {noformat}


          Full stack trace is below. Is there a follow-up patch that needs to be applied as well?

          [1] eZPublishCoreBaseExceptionsNotFoundException: Could not find 'location' with identifier
          at n/a
          in /var/www/vendor/ezsystems/ezpublish-kernel/eZ/Publish/Core/Persistence/Legacy/Content/Location/Gateway/EzcDatabase.php line 79

          at eZPublishCorePersistenceLegacyContentLocationGatewayEzcDatabase->getBasicNodeData(null)
          in /var/www/vendor/ezsystems/ezpublish-kernel/eZ/Publish/Core/Persistence/Legacy/Content/Location/Gateway/ExceptionConversion.php line 56

          at eZPublishCorePersistenceLegacyContentLocationGatewayExceptionConversion->getBasicNodeData(null)
          in /var/www/vendor/ezsystems/ezpublish-kernel/eZ/Publish/Core/Persistence/Legacy/Content/Location/Handler.php line 100

          at eZPublishCorePersistenceLegacyContentLocationHandler->load(null)
          in /var/www/vendor/ezsystems/ezpublish-kernel/eZ/Publish/Core/Persistence/Cache/LocationHandler.php line 32

          at eZPublishCorePersistenceCacheLocationHandler->load(null)
          in /var/www/vendor/ezsystems/ezpublish-kernel/eZ/Publish/Core/Repository/LocationService.php line 205

          at eZPublishCoreRepositoryLocationService->loadLocation(null)
          in /var/www/vendor/ezsystems/ezpublish-kernel/eZ/Publish/Core/SignalSlot/LocationService.php line 102

          at eZPublishCoreSignalSlotLocationService->loadLocation(null)
          in /var/www/src/Hootsuite/BaseBundle/Controller/WebinarsController.php line 925

          at HootsuiteBaseBundleControllerWebinarsController->webinarEnterpriseAction(null, 'full', true, array('content' => object(Content), 'location' => object(Location), 'isPreview' => true))
          in line

          at call_user_func_array(array(object(WebinarsController), 'webinarEnterpriseAction'), array(null, 'full', true, array('content' => object(Content), 'location' => object(Location), 'isPreview' => true)))
          in /var/www/ezpublish/bootstrap.php.cache line 2889

          at SymfonyComponentHttpKernelHttpKernel->handleRaw(object(Request), '2')
          in /var/www/ezpublish/bootstrap.php.cache line 2863

          at SymfonyComponentHttpKernelHttpKernel->handle(object(Request), '2', true)
          in /var/www/ezpublish/bootstrap.php.cache line 2992

          at SymfonyComponentHttpKernelDependencyInjectionContainerAwareHttpKernel->handle(object(Request), '2')
          in /var/www/vendor/ezsystems/ezpublish-kernel/eZ/Publish/Core/MVC/Symfony/Controller/Content/PreviewController.php line 89

          at eZPublishCoreMVCSymfonyControllerContentPreviewController->previewContentAction('3461', '1', 'eng-US', 'eng')
          in line

          at call_user_func_array(array(object(PreviewController), 'previewContentAction'), array('3461', '1', 'eng-US', 'eng'))
          in /var/www/ezpublish/bootstrap.php.cache line 2889

          at SymfonyComponentHttpKernelHttpKernel->handleRaw(object(Request), '1')
          in /var/www/ezpublish/bootstrap.php.cache line 2863

          at SymfonyComponentHttpKernelHttpKernel->handle(object(Request), '1', true)
          in /var/www/ezpublish/bootstrap.php.cache line 2992

          at SymfonyComponentHttpKernelDependencyInjectionContainerAwareHttpKernel->handle(object(Request), '1', true)
          in /var/www/ezpublish/bootstrap.php.cache line 2272

          at SymfonyComponentHttpKernelKernel->handle(object(Request), '1', true)
          in /var/www/vendor/ezsystems/ezpublish-kernel/eZ/Bundle/EzPublishCoreBundle/Kernel.php line 62

          at eZBundleEzPublishCoreBundleKernel->handle(object(Request))
          in /var/www/web/index.php line 64
          {noformat}
          There appears to be a regression in the eZ Publish 5.2 version of the patch from issue https://jira.ez.no/browse/EZP-22781, that occurs in the conditions described below.

          h4. Steps to reproduce:

          1. Prepare an eZ Publish 5.2 installation, fully patched;
          2. Install eZ Publish 5.2 patch from https://jira.ez.no/browse/EZP-22781;
          3. Preview *the first draft* of a page that is being served by a new stack full view template with an Image attribute. If the page is published preview doesn't crash. The problem appears to be happening if you have a custom controller that is trying to access the location ID. In the example below, it is the line that is trying loadLocation() that is failing:

          {noformat}
          public function webinarEnterpriseAction( $locationId, $viewType, $layout = false, array $params = array() ) {
          $params += array();

          $repository = $this->getRepository();
          $location = $repository->getLocationService()->loadLocation( $locationId );
          $content = $repository->getContentService()->loadContentByContentInfo( $location->contentInfo );

          $params['tz_dates'] = $this->globalWebinarDates( $content );

          // Forward the request to the Default Controller
          $response = $this->get( 'ez_content' )->viewLocation( $locationId, $viewType, $layout, $params );

          return $response;
          }
          {noformat}

          In the customer's case, it happens that their site has an Image attribute on that content class. Therefore, to test you just have to create a custom controller and try to access the location ID. It will always crash on a first draft:

          {noformat}


          Full stack trace is below. Is there a follow-up patch that needs to be applied as well?

          [1] eZPublishCoreBaseExceptionsNotFoundException: Could not find 'location' with identifier
          at n/a
          in /var/www/vendor/ezsystems/ezpublish-kernel/eZ/Publish/Core/Persistence/Legacy/Content/Location/Gateway/EzcDatabase.php line 79

          at eZPublishCorePersistenceLegacyContentLocationGatewayEzcDatabase->getBasicNodeData(null)
          in /var/www/vendor/ezsystems/ezpublish-kernel/eZ/Publish/Core/Persistence/Legacy/Content/Location/Gateway/ExceptionConversion.php line 56

          at eZPublishCorePersistenceLegacyContentLocationGatewayExceptionConversion->getBasicNodeData(null)
          in /var/www/vendor/ezsystems/ezpublish-kernel/eZ/Publish/Core/Persistence/Legacy/Content/Location/Handler.php line 100

          at eZPublishCorePersistenceLegacyContentLocationHandler->load(null)
          in /var/www/vendor/ezsystems/ezpublish-kernel/eZ/Publish/Core/Persistence/Cache/LocationHandler.php line 32

          at eZPublishCorePersistenceCacheLocationHandler->load(null)
          in /var/www/vendor/ezsystems/ezpublish-kernel/eZ/Publish/Core/Repository/LocationService.php line 205

          at eZPublishCoreRepositoryLocationService->loadLocation(null)
          in /var/www/vendor/ezsystems/ezpublish-kernel/eZ/Publish/Core/SignalSlot/LocationService.php line 102

          at eZPublishCoreSignalSlotLocationService->loadLocation(null)
          in /var/www/src/Hootsuite/BaseBundle/Controller/WebinarsController.php line 925

          at HootsuiteBaseBundleControllerWebinarsController->webinarEnterpriseAction(null, 'full', true, array('content' => object(Content), 'location' => object(Location), 'isPreview' => true))
          in line

          at call_user_func_array(array(object(WebinarsController), 'webinarEnterpriseAction'), array(null, 'full', true, array('content' => object(Content), 'location' => object(Location), 'isPreview' => true)))
          in /var/www/ezpublish/bootstrap.php.cache line 2889

          at SymfonyComponentHttpKernelHttpKernel->handleRaw(object(Request), '2')
          in /var/www/ezpublish/bootstrap.php.cache line 2863

          at SymfonyComponentHttpKernelHttpKernel->handle(object(Request), '2', true)
          in /var/www/ezpublish/bootstrap.php.cache line 2992

          at SymfonyComponentHttpKernelDependencyInjectionContainerAwareHttpKernel->handle(object(Request), '2')
          in /var/www/vendor/ezsystems/ezpublish-kernel/eZ/Publish/Core/MVC/Symfony/Controller/Content/PreviewController.php line 89

          at eZPublishCoreMVCSymfonyControllerContentPreviewController->previewContentAction('3461', '1', 'eng-US', 'eng')
          in line

          at call_user_func_array(array(object(PreviewController), 'previewContentAction'), array('3461', '1', 'eng-US', 'eng'))
          in /var/www/ezpublish/bootstrap.php.cache line 2889

          at SymfonyComponentHttpKernelHttpKernel->handleRaw(object(Request), '1')
          in /var/www/ezpublish/bootstrap.php.cache line 2863

          at SymfonyComponentHttpKernelHttpKernel->handle(object(Request), '1', true)
          in /var/www/ezpublish/bootstrap.php.cache line 2992

          at SymfonyComponentHttpKernelDependencyInjectionContainerAwareHttpKernel->handle(object(Request), '1', true)
          in /var/www/ezpublish/bootstrap.php.cache line 2272

          at SymfonyComponentHttpKernelKernel->handle(object(Request), '1', true)
          in /var/www/vendor/ezsystems/ezpublish-kernel/eZ/Bundle/EzPublishCoreBundle/Kernel.php line 62

          at eZBundleEzPublishCoreBundleKernel->handle(object(Request))
          in /var/www/web/index.php line 64
          {noformat}
          Nuno Oliveira (Inactive) made changes -
          Status Open [ 1 ] Confirmed [ 10037 ]
          Nuno Oliveira (Inactive) made changes -
          Link This issue relates to EZP-22781 [ EZP-22781 ]
          Hide
          Peter Keung added a comment - - edited

          As pointed out by Philippe, there is some new handling of a "fake" location object in preview mode:
          https://github.com/ezsystems/ezpublish-kernel/blob/master/eZ/Publish/Core/Helper/ContentPreviewHelper.php#L114

          Tracking that down and its related changes (and patching 5.2 with those changes) seem to prevent preview from completely crashing on the first draft. But then the issue is that neither the custom controller nor the custom template specified in override rules will match if they get caught by an override for location ID 2.

          Show
          Peter Keung added a comment - - edited As pointed out by Philippe, there is some new handling of a "fake" location object in preview mode: https://github.com/ezsystems/ezpublish-kernel/blob/master/eZ/Publish/Core/Helper/ContentPreviewHelper.php#L114 Tracking that down and its related changes (and patching 5.2 with those changes) seem to prevent preview from completely crashing on the first draft. But then the issue is that neither the custom controller nor the custom template specified in override rules will match if they get caught by an override for location ID 2.
          Paulo Lopes (Inactive) made changes -
          Status Confirmed [ 10037 ] InputQ [ 10001 ]
          André Rømcke made changes -
          Rank Ranked higher
          Hide
          André Rømcke added a comment -

          Currently discussing introducing a light version of Location drafts for this: https://github.com/ezsystems/ezpublish-kernel/pull/1082

          Show
          André Rømcke added a comment - Currently discussing introducing a light version of Location drafts for this: https://github.com/ezsystems/ezpublish-kernel/pull/1082
          Hide
          André Rømcke added a comment - - edited

          Peter: After some discussions we can'f find a way to expose Location drafts without having to model it properly, so we will have to tackle that as a full story given it is a new feature for Platform stack.

          Do you see anything we can do short term for you to improve on this issue?

          Show
          André Rømcke added a comment - - edited Peter: After some discussions we can'f find a way to expose Location drafts without having to model it properly, so we will have to tackle that as a full story given it is a new feature for Platform stack. Do you see anything we can do short term for you to improve on this issue?
          Hide
          Peter Keung added a comment -

          Thanks for the update. At the moment we're trying to get editors to publish hidden and then preview. This messes up our approval workflow but is a short-term solution. I guess the question is how we can get this prioritized so that when we upgrade to 5.4 or very soon after we have a patch for it?

          Show
          Peter Keung added a comment - Thanks for the update. At the moment we're trying to get editors to publish hidden and then preview. This messes up our approval workflow but is a short-term solution. I guess the question is how we can get this prioritized so that when we upgrade to 5.4 or very soon after we have a patch for it?
          Hide
          André Rømcke added a comment -

          Ok, the solution we end up with might require you to know that the location is a draft before using API (if you have ContentInfo you know, but if you only have locationId you don't), kind of like in legacy where drafts where done over eZNodeAssignment.

          Which brings a question; was this caused by use of embed tags using location id? If so any reason why the foreign location is not published yet? Cause if they are not I'm wondering how this worked in legacy.

          Show
          André Rømcke added a comment - Ok, the solution we end up with might require you to know that the location is a draft before using API (if you have ContentInfo you know, but if you only have locationId you don't), kind of like in legacy where drafts where done over eZNodeAssignment. Which brings a question; was this caused by use of embed tags using location id? If so any reason why the foreign location is not published yet? Cause if they are not I'm wondering how this worked in legacy.
          Hide
          Peter Keung added a comment -

          Hi André,

          No, it's not because of embed tags. The failure to load the location is related to the page that you are on. In the example in the issue description, we need to operate on some of the data in the content object but the controller is only passed the location ID and thus has to fetch the location to be able to get at the fields.

          A lesser issue is if you use the location in Twig to generate Share links or a canonical URL for example. Again, in that case we are just trying to access the location of the page you are on.

          Show
          Peter Keung added a comment - Hi André, No, it's not because of embed tags. The failure to load the location is related to the page that you are on. In the example in the issue description, we need to operate on some of the data in the content object but the controller is only passed the location ID and thus has to fetch the location to be able to get at the fields. A lesser issue is if you use the location in Twig to generate Share links or a canonical URL for example. Again, in that case we are just trying to access the location of the page you are on.
          André Rømcke made changes -
          Issue Type Bug [ 1 ] Improvement [ 4 ]
          André Rømcke made changes -
          Issue Type Improvement [ 4 ] Bug [ 1 ]
          Hide
          André Rømcke added a comment - - edited

          Added JIRA link to the story for location drafts support (EZP-22314), it is in the backlog and will be handled in a future version.

          As for this issue we will have to handle it using documentation.

          @Doc Additional doc needs to be added for content preview mentioning that locations of the previewed content are not available before the content has been published. This is similarly to legacy where they are only available as eZNodeAssigments however not part of the "eZContentObjectTreeNode" structure.

          Peter: As for missing back ports to 5.2, anything needed here or is your local install patched and fine until you migrate to one of the composer based LTS releases (5.3 or 5.4) ?

          Show
          André Rømcke added a comment - - edited Added JIRA link to the story for location drafts support ( EZP-22314 ), it is in the backlog and will be handled in a future version. As for this issue we will have to handle it using documentation. @Doc Additional doc needs to be added for content preview mentioning that locations of the previewed content are not available before the content has been published. This is similarly to legacy where they are only available as eZNodeAssigments however not part of the "eZContentObjectTreeNode" structure. Peter: As for missing back ports to 5.2, anything needed here or is your local install patched and fine until you migrate to one of the composer based LTS releases (5.3 or 5.4) ?
          André Rømcke made changes -
          Link This issue relates to EZP-22314 [ EZP-22314 ]
          André Rømcke made changes -
          Component/s Documentation [ 10793 ]
          André Rømcke made changes -
          Status InputQ [ 10001 ] Development Acceptance Done [ 10030 ]
          Fix Version/s 5.2 Maintenance [ 12782 ]
          Fix Version/s 5.3.4 [ 13879 ]
          Fix Version/s 5.4.0-rc1 [ 13883 ]
          André Rømcke made changes -
          Summary Preview issue after installing patch from EZP-22781 (possible regression) Preview issue when relying on locations for unpublished content
          Hide
          Peter Keung added a comment -

          I should stress that the problem in practice is not around locations – we could probably check for a valid location. It would be inconvenient, but it's a workaround.

          The bigger problems are that:
          1) You cannot get at a field in the controller without going through the location. This means that any time you want to access a field in the controller, first draft preview will crash. The cause is that you don't have a valid location, but it's not because we are necessarily interested in getting the location. Since data manipulation in templates is more limited now and putting business logic in controllers is the best practice, it is very common to need to access field data in controllers.
          2) Currently, the dummy location is being given location ID 2. This means that you'll always get the override template for location ID 2, rendering the times that preview actually does work for first drafts useless.

          I'd say the missing backport so far for 5.2 is for creating the dummy location, as Philippe pointed out (https://github.com/ezsystems/ezpublish-kernel/blob/master/eZ/Publish/Core/Helper/ContentPreviewHelper.php#L114) although we were able to successfully manually apply it. But that doesn't completely solve the issue

          Show
          Peter Keung added a comment - I should stress that the problem in practice is not around locations – we could probably check for a valid location. It would be inconvenient, but it's a workaround. The bigger problems are that: 1) You cannot get at a field in the controller without going through the location. This means that any time you want to access a field in the controller, first draft preview will crash. The cause is that you don't have a valid location, but it's not because we are necessarily interested in getting the location. Since data manipulation in templates is more limited now and putting business logic in controllers is the best practice, it is very common to need to access field data in controllers. 2) Currently, the dummy location is being given location ID 2. This means that you'll always get the override template for location ID 2, rendering the times that preview actually does work for first drafts useless. I'd say the missing backport so far for 5.2 is for creating the dummy location, as Philippe pointed out ( https://github.com/ezsystems/ezpublish-kernel/blob/master/eZ/Publish/Core/Helper/ContentPreviewHelper.php#L114 ) although we were able to successfully manually apply it. But that doesn't completely solve the issue
          Sarah Haïm-Lubczanski (Inactive) made changes -
          Status Development Acceptance Done [ 10030 ] Documentation [ 10010 ]
          Assignee Sarah Haïm-Lubczanski [ sarah.haim-lubczanski@ez.no ]
          André Rømcke made changes -
          Status Documentation [ 10010 ] InputQ [ 10001 ]
          Assignee Sarah Haïm-Lubczanski [ sarah.haim-lubczanski@ez.no ]
          André Rømcke made changes -
          Fix Version/s Engineering tracked issues [ 11179 ]
          Fix Version/s Customer request [ 11018 ]
          Fix Version/s 5.2 Maintenance [ 12782 ]
          Fix Version/s 5.3.4 [ 13879 ]
          André Rømcke made changes -
          Fix Version/s 5.4.0-rc1 [ 13883 ]
          Sarah Haïm-Lubczanski (Inactive) made changes -
          Status InputQ [ 10001 ] Documentation [ 10010 ]
          Assignee Sarah Haïm-Lubczanski [ sarah.haim-lubczanski@ez.no ]
          Sarah Haïm-Lubczanski (Inactive) made changes -
          Status Documentation [ 10010 ] InputQ [ 10001 ]
          Assignee Sarah Haïm-Lubczanski [ sarah.haim-lubczanski@ez.no ]
          André Rømcke made changes -
          Fix Version/s Customer request [ 11018 ]
          André Rømcke made changes -
          Fix Version/s 5.2 Maintenance [ 12782 ]
          Fix Version/s 5.2 [ 12582 ]
          André Rømcke made changes -
          Fix Version/s 5.2 [ 12582 ]
          André Rømcke made changes -
          Labels SilverBullet
          Hide
          Bertrand Dunogier added a comment - - edited

          Hi Peter. Got a few extra questions.

          In the example given in the issue's description, you use a locationId based controller. Why ? Nothing in the controller actually requires a locationId, does it ? Using a content id instead would work without errors, would it not ?

          This does not bring an answer to generating social links, but maybe it should be done differently...

          Furthermore, about overrides: what's your use-case here ? Overriding based on parent_location_id ? There is no way you're overriding based on the id of the node you're creating, is there ?

          I haven't reached the end of this pit yet, but I don't think we had a real location_id in legacy's versionview on version 1.

          Unless I'm mistaken, the current code requires https://github.com/ezsystems/ezpublish-kernel/pull/986 + https://github.com/ezsystems/ezpublish-kernel/pull/846 to be backported, right ?

          Show
          Bertrand Dunogier added a comment - - edited Hi Peter. Got a few extra questions. In the example given in the issue's description, you use a locationId based controller. Why ? Nothing in the controller actually requires a locationId, does it ? Using a content id instead would work without errors, would it not ? This does not bring an answer to generating social links, but maybe it should be done differently... Furthermore, about overrides: what's your use-case here ? Overriding based on parent_location_id ? There is no way you're overriding based on the id of the node you're creating, is there ? I haven't reached the end of this pit yet, but I don't think we had a real location_id in legacy's versionview on version 1. Unless I'm mistaken, the current code requires https://github.com/ezsystems/ezpublish-kernel/pull/986 + https://github.com/ezsystems/ezpublish-kernel/pull/846 to be backported, right ?
          Hide
          Peter Keung added a comment -

          Hi Bertrand,

          "In the example given in the issue's description, you use a locationId based controller. Why ? Nothing in the controller actually requires a locationId, does it ? Using a content id instead would work without errors, would it not ?
          This does not bring an answer to generating social links, but maybe it should be done differently..."

          You are right – in the controller itself (at least in this case) we do not need the location. We do need it in the templates though. This is a newbie statement, but: we didn't even consider that possibility to use viewContent, because we figured we are in a "node / location full view" context. Don't we then have to take extra care to send the X-Location-Id header for caching (and maybe other things)?

          "Furthermore, about overrides: what's your use-case here ? Overriding based on parent_location_id ? There is no way you're overriding based on the id of the node you're creating, is there ?"

          It's just a standard content type override. Although my comment about an override rule for location ID 2 clashing with the "give the draft location the ID of the root node" is for another case where we are overriding the homepage. In that case I suppose we could use a content ID override instead, but again it is then an issue where the templates crash when you use the location.

          "Unless I'm mistaken, the current code requires https://github.com/ezsystems/ezpublish-kernel/pull/986 + https://github.com/ezsystems/ezpublish-kernel/pull/846 to be backported, right ?"

          At this point I think "upgrade to eZ Publish 5.4" is a reasonable solution, but as far as I understand that still has the same issue(s) for first-draft preview, doesn't it? (That is, any use of the location will crash and a node/location override for ID 2 – if you have one – will be triggered.)

          Show
          Peter Keung added a comment - Hi Bertrand, "In the example given in the issue's description, you use a locationId based controller. Why ? Nothing in the controller actually requires a locationId, does it ? Using a content id instead would work without errors, would it not ? This does not bring an answer to generating social links, but maybe it should be done differently..." You are right – in the controller itself (at least in this case) we do not need the location. We do need it in the templates though. This is a newbie statement, but: we didn't even consider that possibility to use viewContent, because we figured we are in a "node / location full view" context. Don't we then have to take extra care to send the X-Location-Id header for caching (and maybe other things)? "Furthermore, about overrides: what's your use-case here ? Overriding based on parent_location_id ? There is no way you're overriding based on the id of the node you're creating, is there ?" It's just a standard content type override. Although my comment about an override rule for location ID 2 clashing with the "give the draft location the ID of the root node" is for another case where we are overriding the homepage. In that case I suppose we could use a content ID override instead, but again it is then an issue where the templates crash when you use the location. "Unless I'm mistaken, the current code requires https://github.com/ezsystems/ezpublish-kernel/pull/986 + https://github.com/ezsystems/ezpublish-kernel/pull/846 to be backported, right ?" At this point I think "upgrade to eZ Publish 5.4" is a reasonable solution, but as far as I understand that still has the same issue(s) for first-draft preview, doesn't it? (That is, any use of the location will crash and a node/location override for ID 2 – if you have one – will be triggered.)
          Hide
          Bertrand Dunogier added a comment - - edited

          Yes, those are still true.

          To be frank, I'm thinking that putting location ID 2 in there was an error. It creates more issues than it fixes... You can check the last comment André wrote when closing https://github.com/ezsystems/ezpublish-kernel/pull/1082, it's quite interesting, I believe.

          Regarding Location vs Content, I think that we are touching something here.

          Why, when and how we use locations might actually be the question here. Your main argument, as far as I can tell, is the X-Location-id header. But pragmatically, using the main location won't hurt here, as smart cache rules always clear cache for all locations of a published content.

          Another use-case for a specific location would be to render it depending on where the content is located. But I'd say that overall, embedded renders don't really need the location, especially since content relations are at Content level, not Location level.

          The most problematic case would actually be content with multiple locations, when the locations do have a business meaning. If you have use-cases in mind, I'm interested.

          In any case, regarding the issue at hand, I don't see real benefits with the fake lode_id. In version 1, a content does NOT have a location_id. Whatever we provide will be wrong, as there isn't one in the database. We could otoh care about the parent_location_id, as overriding on that does make sense, right ? André's comment is about this particular aspect, the changes to locations that are "programmed" with a publishing operation (creating a node, moving one, removing one...).

          Show
          Bertrand Dunogier added a comment - - edited Yes, those are still true. To be frank, I'm thinking that putting location ID 2 in there was an error. It creates more issues than it fixes... You can check the last comment André wrote when closing https://github.com/ezsystems/ezpublish-kernel/pull/1082 , it's quite interesting, I believe. Regarding Location vs Content, I think that we are touching something here. Why, when and how we use locations might actually be the question here. Your main argument, as far as I can tell, is the X-Location-id header. But pragmatically, using the main location won't hurt here, as smart cache rules always clear cache for all locations of a published content. Another use-case for a specific location would be to render it depending on where the content is located. But I'd say that overall, embedded renders don't really need the location, especially since content relations are at Content level, not Location level. The most problematic case would actually be content with multiple locations, when the locations do have a business meaning. If you have use-cases in mind, I'm interested. In any case, regarding the issue at hand, I don't see real benefits with the fake lode_id. In version 1, a content does NOT have a location_id. Whatever we provide will be wrong, as there isn't one in the database. We could otoh care about the parent_location_id, as overriding on that does make sense, right ? André's comment is about this particular aspect, the changes to locations that are "programmed" with a publishing operation (creating a node, moving one, removing one...).
          Bertrand Dunogier made changes -
          Status InputQ [ 10001 ] Development [ 3 ]
          Assignee Bertrand Dunogier [ bertrand.dunogier@ez.no ]
          Bertrand Dunogier made changes -
          Comment [ Oh, one more thing (tm).

          I'm thinking that the fake location_id in the LocationDraft object does not help at all, in the end. I'd rather remove it from the backport. What do you think (I'd rather focus on getting the parent there...) ? ]
          Bertrand Dunogier made changes -
          Status Development [ 3 ] Development review [ 10006 ]
          Hide
          Bertrand Dunogier added a comment -

          The fake location thing has been backported to the 5.2 branch, and will be shipped in a further SP.

          Show
          Bertrand Dunogier added a comment - The fake location thing has been backported to the 5.2 branch, and will be shipped in a further SP.
          Bertrand Dunogier made changes -
          Status Development review [ 10006 ] Development Review done [ 10028 ]
          Bertrand Dunogier made changes -
          Status Development Review done [ 10028 ] Documentation done [ 10011 ]
          Hide
          Peter Keung added a comment -

          About the problem of a fake node ID – it sounds like we can work around that in some way.

          About the X-Location-Id header, I was just trying to come up with some example about why we would need the location in a controller. I suppose the question is, when do we need to use the location at all in a full view anywhere, whether in a controller or a template?

          • Location ID. Off the top of my head, I can remember passing this information in a meta tag to be sent to some third party JS tool. Or sometimes I've seen it used in CSS classes for the purposes of page-specific style overrides or for some special logic in menu highlighting. Whether or not those are the best examples, there will be reasons to use the location ID.
          • URL alias: definitely a stronger use case as we have share tools and canonical tags, among other things.

          In both of the above cases, it's OK if they produce something like NULL on preview. But they shouldn't crash preview.

          Perhaps you could instruct developers to always wrap their location-specific logic in "if" or "try/catch" statements but I don't think that's reasonable when you're trying to access the very location you are viewing.

          How does legacy do it? Maybe legacy "benefits" from silent errors and direct aka non-service oriented access between a node and an object.

          Show
          Peter Keung added a comment - About the problem of a fake node ID – it sounds like we can work around that in some way. About the X-Location-Id header, I was just trying to come up with some example about why we would need the location in a controller. I suppose the question is, when do we need to use the location at all in a full view anywhere, whether in a controller or a template? Location ID. Off the top of my head, I can remember passing this information in a meta tag to be sent to some third party JS tool. Or sometimes I've seen it used in CSS classes for the purposes of page-specific style overrides or for some special logic in menu highlighting. Whether or not those are the best examples, there will be reasons to use the location ID. URL alias: definitely a stronger use case as we have share tools and canonical tags, among other things. In both of the above cases, it's OK if they produce something like NULL on preview. But they shouldn't crash preview. Perhaps you could instruct developers to always wrap their location-specific logic in "if" or "try/catch" statements but I don't think that's reasonable when you're trying to access the very location you are viewing. How does legacy do it? Maybe legacy "benefits" from silent errors and direct aka non-service oriented access between a node and an object.
          Hide
          Peter Keung added a comment -

          Sorry that this is turning into a forum post. But I have to ask: how exactly can we create an override rule for a full location view based on content ID but then tell it to use a viewContent( $contentId, $viewType, $layout = false, array $params = array() ) signature instead of a viewLocation( $locationId, $viewType, $layout = false, array $params = array() ) signature? One of the original problems we had was that to get at content fields in a custom controller for a full location view we had to go through the location ID. All I can think of it to use a pre-content view listener, which seems a bit overkill.

          Show
          Peter Keung added a comment - Sorry that this is turning into a forum post. But I have to ask: how exactly can we create an override rule for a full location view based on content ID but then tell it to use a viewContent( $contentId, $viewType, $layout = false, array $params = array() ) signature instead of a viewLocation( $locationId, $viewType, $layout = false, array $params = array() ) signature? One of the original problems we had was that to get at content fields in a custom controller for a full location view we had to go through the location ID. All I can think of it to use a pre-content view listener, which seems a bit overkill.
          Hide
          Bertrand Dunogier added a comment -

          Sorry that this is turning into a forum post.

          My only regret, and it's not a big one, is that it would be better on the forums. More people could participate

          Well, you've reached the same no-man's-land I've came across a couple minutes ago. I'm thinking (that's really personal) that we have a flaw in our domain, more specifically in content vs. location... our first class citizen is the content. We always have a Content. We don't always have a Location, and we actually have in mind Location less Content, as a feature.

          If we replaced the locationView controller with an optional location argument for the contentView controller, wouldn't it fix most issues ?

          Show
          Bertrand Dunogier added a comment - Sorry that this is turning into a forum post. My only regret, and it's not a big one, is that it would be better on the forums. More people could participate Well, you've reached the same no-man's-land I've came across a couple minutes ago. I'm thinking (that's really personal) that we have a flaw in our domain, more specifically in content vs. location... our first class citizen is the content. We always have a Content. We don't always have a Location, and we actually have in mind Location less Content, as a feature. If we replaced the locationView controller with an optional location argument for the contentView controller, wouldn't it fix most issues ?
          Hide
          Bertrand Dunogier added a comment -

          My first reaction, from a higher perspective is that we might have a context problem here. Is everything supposed to work in preview ? I'm thinking that maybe a context variable, passed to all content templates, and set to 'preview', might make sense. Don't you think so ? We could alter the layout for preview only, so that rendering makes sense from the perspective of an editor previewing content.

          Anyway, use-cases:

          • url-alias: so what you need is actually an URL alias, not a location. I guess it makes sense that an editor would expect the URL to be previewable as well. Not sure we can easily pull that one out... I would nonetheless say that it ain't critical for editors, and could be handled with the context thing above, right ?
          • location id for JS: what would the JS tool need this ID for in preview ? Would they work (again, in a preview context) with the content id ?
          • menu highlighting: it is very unlikely that a business rule uses the id of a location that is being created, right ? We are working on a fix that would I think cover the use-case here: EZP-23784 (you would have both depth and parent location id valid for the main location. Let us know what you think, it has a pull-request already).

          Oh, one note: I have created a preview enhancement suggestion, do not hesitate to comment.

          Show
          Bertrand Dunogier added a comment - My first reaction, from a higher perspective is that we might have a context problem here. Is everything supposed to work in preview ? I'm thinking that maybe a context variable, passed to all content templates, and set to 'preview', might make sense. Don't you think so ? We could alter the layout for preview only, so that rendering makes sense from the perspective of an editor previewing content. Anyway, use-cases: url-alias: so what you need is actually an URL alias, not a location. I guess it makes sense that an editor would expect the URL to be previewable as well. Not sure we can easily pull that one out... I would nonetheless say that it ain't critical for editors, and could be handled with the context thing above, right ? location id for JS: what would the JS tool need this ID for in preview ? Would they work (again, in a preview context) with the content id ? menu highlighting: it is very unlikely that a business rule uses the id of a location that is being created, right ? We are working on a fix that would I think cover the use-case here: EZP-23784 (you would have both depth and parent location id valid for the main location . Let us know what you think, it has a pull-request already). Oh, one note: I have created a preview enhancement suggestion , do not hesitate to comment.
          Ricardo Correia (Inactive) made changes -
          Status Documentation done [ 10011 ] QA [ 10008 ]
          Assignee Bertrand Dunogier [ bertrand.dunogier@ez.no ] Ricardo Correia [ ricardo.correia@ez.no ]
          Ricardo Correia (Inactive) made changes -
          Flagged Impediment [ 10000 ]
          Bertrand Dunogier made changes -
          Link This issue relates to EZP-23793 [ EZP-23793 ]
          Hide
          Bertrand Dunogier added a comment - - edited

          When you say you can preview using viewLocation, what do you mean exactly ?

          The PreviewController will use whatever override rules are defined, and will use either the LocationView or the ContentView controller depending on those rules. When the LocationView controller is used, it can NOT work as expected in any of the stable versions when previewing the first version of a content:

          • either we preview the wrong node id (5.3, 5.4)
          • either we get a fatal error because it is set to null.

          Furthermore, a ContentView controller or template may expect the location to be here, and use it as an argument, to sub-requests or functions. It is done like this in the Article and Blog post templates (and controller for blog post).

          There might be something I've missed, but I'd say I'm pretty close to the bottom of the pit.

          Show
          Bertrand Dunogier added a comment - - edited When you say you can preview using viewLocation, what do you mean exactly ? The PreviewController will use whatever override rules are defined, and will use either the LocationView or the ContentView controller depending on those rules. When the LocationView controller is used, it can NOT work as expected in any of the stable versions when previewing the first version of a content: either we preview the wrong node id (5.3, 5.4) either we get a fatal error because it is set to null. Furthermore, a ContentView controller or template may expect the location to be here, and use it as an argument, to sub-requests or functions. It is done like this in the Article and Blog post templates (and controller for blog post). There might be something I've missed, but I'd say I'm pretty close to the bottom of the pit.
          Bertrand Dunogier made changes -
          Link This issue relates to EZP-23793 [ EZP-23793 ]
          Hide
          Ricardo Correia (Inactive) added a comment - - edited

          Hi Bertrand,
          I haven't tested using blog posts. I used content classes similar to the article class, and another one with a text line and an image attribute.
          To test viewLocation I injected a call to a custom controller, and the controller will return the content based on the location. This works if I provide the location ID of an already existing content.
          What content ID should I use? The ID of an existing content, or the ID of the content I'm creating (which has a single version still in draft).
          AFAIK content that hasn't been published yet doesn't have a location ID yet, correct?

          Is not this the right direction?
          Am I missing something here?

          BTW, I'm testing using version 5.2 fully patched.

          Show
          Ricardo Correia (Inactive) added a comment - - edited Hi Bertrand, I haven't tested using blog posts. I used content classes similar to the article class, and another one with a text line and an image attribute. To test viewLocation I injected a call to a custom controller, and the controller will return the content based on the location. This works if I provide the location ID of an already existing content. What content ID should I use? The ID of an existing content, or the ID of the content I'm creating (which has a single version still in draft). AFAIK content that hasn't been published yet doesn't have a location ID yet, correct? Is not this the right direction? Am I missing something here? BTW, I'm testing using version 5.2 fully patched.
          Hide
          Peter Keung added a comment -

          "I'm thinking that maybe a context variable, passed to all content templates, and set to 'preview', might make sense. Don't you think so ? We could alter the layout for preview only, so that rendering makes sense from the perspective of an editor previewing content."

          Perhaps that is possible, but the bigger problem is that you currently have to go through the Location ID to get at anything from the Content, such as its fields, in a custom view controller. So skipping the lookup of the fields is too big of a break.

          Also, I think in practice, you will end up wrapping all your uses of the Location in "if" statements checking whether the context is preview. Having the context would be nice to have for some cases, such as putting some placeholder text for an ad for example. BUT from a dev UX standpoint, having to check for the "preview" context when using the Location in some way is not very nice. It would be better if any uses of the Location just silently failed.

          Show
          Peter Keung added a comment - "I'm thinking that maybe a context variable, passed to all content templates, and set to 'preview', might make sense. Don't you think so ? We could alter the layout for preview only, so that rendering makes sense from the perspective of an editor previewing content." Perhaps that is possible, but the bigger problem is that you currently have to go through the Location ID to get at anything from the Content, such as its fields, in a custom view controller. So skipping the lookup of the fields is too big of a break. Also, I think in practice, you will end up wrapping all your uses of the Location in "if" statements checking whether the context is preview. Having the context would be nice to have for some cases, such as putting some placeholder text for an ad for example. BUT from a dev UX standpoint, having to check for the "preview" context when using the Location in some way is not very nice. It would be better if any uses of the Location just silently failed.
          Hide
          Bertrand Dunogier added a comment -

          Perhaps that is possible, but the bigger problem is that you currently have to go through the Location ID to get at anything from the Content, such as its fields, in a custom view controller. So skipping the lookup of the fields is too big of a break.

          That is because you use the LocationView controller (location_view rule). If you use the ContentView controller, your main argument is the ContentId, not the LocationId.

          I've been through every content view rule in ezdemo.yml, and saw only two cases where the actual location was required (it was used in much more than that of course).

          But I'm still working on that.

          Show
          Bertrand Dunogier added a comment - Perhaps that is possible, but the bigger problem is that you currently have to go through the Location ID to get at anything from the Content, such as its fields, in a custom view controller. So skipping the lookup of the fields is too big of a break. That is because you use the LocationView controller (location_view rule). If you use the ContentView controller, your main argument is the ContentId, not the LocationId. I've been through every content view rule in ezdemo.yml, and saw only two cases where the actual location was required (it was used in much more than that of course). But I'm still working on that.
          Ricardo Correia (Inactive) made changes -
          Link This issue relates to EZP-23807 [ EZP-23807 ]
          Hide
          Ricardo Correia (Inactive) added a comment -

          QA approved.
          This fix solves the Could not find 'location' with identifier '' error.
          Another issue has been raised from this one: https://jira.ez.no/browse/EZP-23807

          Show
          Ricardo Correia (Inactive) added a comment - QA approved. This fix solves the Could not find 'location' with identifier '' error. Another issue has been raised from this one: https://jira.ez.no/browse/EZP-23807
          Ricardo Correia (Inactive) made changes -
          Assignee Ricardo Correia [ ricardo.correia@ez.no ]
          Status QA [ 10008 ] Closed [ 6 ]
          Resolution Fixed [ 1 ]
          André Rømcke made changes -
          Workflow eZ Engineering Scrumban Workflow [ 65063 ] EZ* Development Workflow [ 85489 ]
          Alex Schuster made changes -
          Workflow EZ* Development Workflow [ 85489 ] EZEE Development Workflow [ 124325 ]
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Confirmed Confirmed
          6m 19s 1 nuno.oliveira@ez.no 05/Nov/14 10:58 PM
          Confirmed Confirmed InputQ InputQ
          9h 32m 1 Paulo Lopes (Inactive) 06/Nov/14 8:31 AM
          InputQ InputQ Removed Status Removed Status
          8d 3h 18m 1 André Rømcke 14/Nov/14 11:49 AM
          Removed Status Removed Status Documentation Documentation
          3d 22h 13m 1 sarah.haim-lubczanski@ez.no 18/Nov/14 10:02 AM
          InputQ InputQ Documentation Documentation
          1d 15h 57m 1 sarah.haim-lubczanski@ez.no 03/Dec/14 11:00 AM
          Documentation Documentation InputQ InputQ
          15d 16h 8m 2 sarah.haim-lubczanski@ez.no 05/Dec/14 6:07 PM
          InputQ InputQ Development Development
          4d 21h 26m 1 Bertrand Dunogier 10/Dec/14 3:33 PM
          Development Development Development Review Development Review
          21s 1 Bertrand Dunogier 10/Dec/14 3:34 PM
          Development Review Development Review Development Review done Development Review done
          9m 27s 1 Bertrand Dunogier 10/Dec/14 3:43 PM
          Development Review done Development Review done Documentation Review done Documentation Review done
          8s 1 Bertrand Dunogier 10/Dec/14 3:43 PM
          Documentation Review done Documentation Review done QA QA
          1d 21m 1 ricardo.correia@ez.no 11/Dec/14 4:04 PM
          QA QA Closed Closed
          4d 23h 47m 1 ricardo.correia@ez.no 16/Dec/14 3:51 PM

            People

            • Assignee:
              Unassigned
              Reporter:
              Nuno Oliveira (Inactive)
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: