-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[Improvement]: [SeoBundle] Redirects - allow redirect target to be a data object or an asset #18293
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
base: 12.x
Are you sure you want to change the base?
Conversation
Review Checklist
|
9af1c00
to
ecb0339
Compare
|
||
protected ?string $target = null; | ||
|
||
protected ?string $targetType = null; |
There was a problem hiding this comment.
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';
}
12ec610
to
6130b75
Compare
6130b75
to
4bdd0b8
Compare
|
return Ext.util.Format.htmlEncode(value); | ||
} | ||
}, | ||
{text: t("target_type"), flex: 70, sortable: true, dataIndex: 'targetType', hidden: true, editor: { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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))
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
- system validates - if element is not found, target type is reset to
Additionally, updated the migration, converted targetType
column to an enum.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Moved translation from DB to YAML file
- Updated migration - added column existence checks
There was a problem hiding this comment.
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.
e42a247
to
fb41f46
Compare
6d34f2e
to
bdacaae
Compare
bdacaae
to
adcdec8
Compare
|
Resolves #15398
Assets are also included, meaning that now user can set any element (
object
,asset
ordocument
) as redirect target.