Skip to content

Conversation

dolmit-tanel-paaro
Copy link
Contributor

Resolves #15398

Assets are also included, meaning that now user can set any element (object, asset or document) as redirect target.

Copy link

Review Checklist

  • Target branch (11.5 for bug fixes, others 12.x)
  • Tests (if it's testable code, there should be a test for it - get help)
  • Docs (every functionality needs to be documented, see here)
  • Migration incl. install.sql (e.g. if the database schema changes, ...)
  • Upgrade notes (deprecations, important information, migration hints, ...)
  • Label
  • Milestone

@dolmit-tanel-paaro dolmit-tanel-paaro force-pushed the feature/seo-bundle-product-and-asset-redirects branch from 9af1c00 to ecb0339 Compare April 14, 2025 11:40
@dolmit-tanel-paaro dolmit-tanel-paaro changed the title allow object and asset as redirect target [Improvement]: Allow redirects to data objects and assets Apr 14, 2025

protected ?string $target = null;

protected ?string $targetType = null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps should introduce an enum for element types. For example:

enum ElementType: string
{
    case Object = 'object';
    case Asset = 'asset';
    case Document = 'document';
}

@dolmit-tanel-paaro dolmit-tanel-paaro force-pushed the feature/seo-bundle-product-and-asset-redirects branch 4 times, most recently from 12ec610 to 6130b75 Compare April 15, 2025 09:29
@dolmit-tanel-paaro dolmit-tanel-paaro force-pushed the feature/seo-bundle-product-and-asset-redirects branch from 6130b75 to 4bdd0b8 Compare April 15, 2025 09:41
Copy link

@dolmit-tanel-paaro dolmit-tanel-paaro changed the title [Improvement]: Allow redirects to data objects and assets [Improvement]: [SeoBundle] Allow redirects to data objects and assets Apr 15, 2025
@dolmit-tanel-paaro dolmit-tanel-paaro changed the title [Improvement]: [SeoBundle] Allow redirects to data objects and assets [Improvement]: [SeoBundle] Redirects - allow redirect target to be a data object or an asset Apr 15, 2025
@ghost ghost added the Pimcore:ToDo label Apr 28, 2025
@robertSt7 robertSt7 self-assigned this Aug 26, 2025
return Ext.util.Format.htmlEncode(value);
}
},
{text: t("target_type"), flex: 70, sortable: true, dataIndex: 'targetType', hidden: true, editor: {
Copy link
Contributor

Choose a reason for hiding this comment

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

@dolmit-tanel-paaro I am not sure why do you hide this column.

When I drop an Asset or DataObject on the target field it shows the path, but when I update the record the target field is blank. Could you please take a look at this? Thanks

Copy link

@dolmit-tpaaro dolmit-tpaaro Aug 27, 2025

Choose a reason for hiding this comment

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

I'm not sure either as to why I hid the column :D I can change that, not a problem.

The updating issue, though, I can't understand it. For example, when dragging an object, the system run into an error which are caused by NULL userModification and userOwner values. Is the system supposed to initialize these values automatically when updating the row? If so, then I don't see how.

URL: /admin/bundle/seo/redirects/list?xaction=update&_dc=1756286087595
Method: POST
Message: Pimcore\Bundle\SeoBundle\Model\Redirect::setUserModification(): Argument #1 ($userModification) must be of type int, null given, called in /var/www/html/vendor/pimcore/pimcore/lib/Model/AbstractModel.php on line 189
Trace: 
in /var/www/html/vendor/pimcore/pimcore/bundles/SeoBundle/src/Model/Redirect.php:406
#0 /var/www/html/vendor/pimcore/pimcore/lib/Model/AbstractModel.php(189): Pimcore\Bundle\SeoBundle\Model\Redirect->setUserModification(NULL)
#1 /var/www/html/vendor/pimcore/pimcore/lib/Model/AbstractModel.php(173): Pimcore\Model\AbstractModel->setValue('userModificatio...', NULL, false)
#2 /var/www/html/vendor/pimcore/pimcore/bundles/SeoBundle/src/Controller/RedirectsController.php(114): Pimcore\Model\AbstractModel->setValues(Array)
#3 /var/www/html/vendor/symfony/http-kernel/HttpKernel.php(181): Pimcore\Bundle\SeoBundle\Controller\RedirectsController->redirectsAction(Object(Symfony\Component\HttpFoundation\Request), Object(Pimcore\Bundle\SeoBundle\Redirect\RedirectHandler))

Copy link

@dolmit-tpaaro dolmit-tpaaro Aug 27, 2025

Choose a reason for hiding this comment

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

Okay, nevermind, turns out the user related issue is fixed in the latest version, was testing on old one.

I figured out the flickering issue - the drag-n-drop action triggered row update twice. We're supposed to use beginEdit and endEdit functions when we want to change multiple grid record fields.

@robertSt7 Please try again and let me know how it goes.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dolmit-tanel-paaro Now the column is visible :) The flickering issue still exists on my side also with documents. Could you please take another look at this? TIA

Copy link

@dolmit-tpaaro dolmit-tpaaro Aug 29, 2025

Choose a reason for hiding this comment

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

Okay, should be solved now. The issue was that when adding a new redirect, system expected target type to be defined.
Target type is not a required value anymore:

  • when creating new redirect, system assumes the target is a document
  • when updating row, user has the freedom to set the target type from dropdown
    • system validates - if element is not found, target type is reset to NULL

Additionally, updated the migration, converted targetType column to an enum.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dolmit-tpaaro Thanks it looks good now. There is only one topic left: Where we should put the translation for targetType? We will discuss this internal the next days

Copy link

@dolmit-tpaaro dolmit-tpaaro Sep 4, 2025

Choose a reason for hiding this comment

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

Right now I included the translation using translations_admin db table because most of the other translations are not in the YAML files.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dolmit-tpaaro I understand your approach, but we decided to move all new translations to the YAML files. Could you please remove it from DB and add it here? Thanks

Choose a reason for hiding this comment

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

@robertSt7

  • Moved translation from DB to YAML file
  • Updated migration - added column existence checks

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps should introduce an enum for element types. For example:

enum ElementType: string
{
    case Object = 'object';
    case Asset = 'asset';
    case Document = 'document';
}

@dolmit-tpaaro Thanks for doing that, looks good. It would be great if you add the enum type. Then we can merge this.

@dolmit-tanel-paaro dolmit-tanel-paaro force-pushed the feature/seo-bundle-product-and-asset-redirects branch 10 times, most recently from e42a247 to fb41f46 Compare August 29, 2025 10:33
@dolmit-tanel-paaro dolmit-tanel-paaro force-pushed the feature/seo-bundle-product-and-asset-redirects branch 2 times, most recently from 6d34f2e to bdacaae Compare September 5, 2025 10:22
@dolmit-tanel-paaro dolmit-tanel-paaro force-pushed the feature/seo-bundle-product-and-asset-redirects branch from bdacaae to adcdec8 Compare September 5, 2025 10:25
Copy link

sonarqubecloud bot commented Sep 5, 2025

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.

[Improvement]: Allow redirects to data objects
3 participants