Skip to content

[Task] Move sitemaps and redirects to SEO Bundle#14348

Merged
mattamon merged 32 commits into11.xfrom
14260-refactor-sitemaps-and-redirects-bundle
Feb 27, 2023
Merged

[Task] Move sitemaps and redirects to SEO Bundle#14348
mattamon merged 32 commits into11.xfrom
14260-refactor-sitemaps-and-redirects-bundle

Conversation

@martineiber
Copy link
Copy Markdown
Contributor

@martineiber martineiber commented Feb 15, 2023

Changes in this pull request

Resolves #14260

Additional info

Follow-up PR pimcore/demo#417

@martineiber martineiber added this to the 11.0.0 milestone Feb 15, 2023
@github-actions
Copy link
Copy Markdown

Review Checklist

  • Target branch (10.5 for bug fixes, others 11.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

Copy link
Copy Markdown
Contributor

@mattamon mattamon left a comment

Choose a reason for hiding this comment

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

The redirects table should also be installed in the Bundle Installer and not from the Installerbundle

The redirects do not open anymore in the backend.
bundles/SeoBundle/public/js/redirects.js -> You have to also change the namespaces like with the rest of the seo bundle.

bundles/SeoBundle/src/Controller/RedirectsController.php also here the names of the routes need to be changed .

Check the https://github.com/pimcore/product-management/issues/13#RefactoringTips for more info.

You also need to remove the redirect menu from the toolbar.js to the startup.js in the bundle.
If the icon is not used anywhere else you can move it to the icons.css in the bundle too.
Also the handler this.editRedirects need to be moved into the startup.js

  if (user.isAllowed("redirects") && perspectiveCfg.inToolbar("extras.redirects")) {
       extrasItems.push({
           text: t("redirects"),
           iconCls: "pimcore_nav_icon_redirects",
           itemId: 'pimcore_menu_extras_redirects',
           handler: this.editRedirects
       });
   }

* Move Redirect status_codes to SEOBundle.
* Moved Events to SEOBundle
  * Hardlink doDelete
  * Page doDelete
  * URLSlag Change
Moved redirect table install to SEOBundle.
Moved redirect Cleanup task to SEOBundle.
Rename route name.
Moved menu creation to SEOBundle.
# Conflicts:
#	bundles/CoreBundle/src/DependencyInjection/Configuration.php
#	bundles/CoreBundle/src/DependencyInjection/PimcoreCoreExtension.php
#	bundles/SeoBundle/config/services.yaml
#	models/Document/Page.php
@martineiber martineiber force-pushed the 14260-refactor-sitemaps-and-redirects-bundle branch from 7af16c2 to efa03e3 Compare February 20, 2023 15:42
Copy link
Copy Markdown
Contributor

@mattamon mattamon left a comment

Choose a reason for hiding this comment

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

Overall looks fine to me, just some minor changes.

Please check the https://github.com/pimcore/pimcore/blob/11.x/bundles/AdminBundle/public/js/pimcore/document/tree.js#L316, if nodes are moved we ask if redirects should be created.

In the DocumentsController are still some references to the old Redirects.
https://github.com/pimcore/pimcore/blob/11.x/bundles/AdminBundle/src/Controller/Admin/Document/DocumentController.php#L563

Copy link
Copy Markdown
Contributor

@mattamon mattamon left a comment

Choose a reason for hiding this comment

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

Looks fine for me, just a few small concerns that need to be considered.

Copy link
Copy Markdown
Contributor

@mattamon mattamon left a comment

Choose a reason for hiding this comment

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

I uninstalled the bundle, but it is still enabled in the bundles.php
Unfortunately I get the following error now.

image

Can you please check this?

Copy link
Copy Markdown
Contributor

@dvesh3 dvesh3 left a comment

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

@dvesh3 dvesh3 left a comment

Choose a reason for hiding this comment

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

One last thing: right now renaming a document has a checkbox to create a redirect. The logic should be moved to the SeoBundle.
image

@mattamon
Copy link
Copy Markdown
Contributor

@martineiber can you please merge the latest changes and then I think we are good to merge. :)

# Conflicts:
#	bundles/CoreBundle/config/pimcore/default.yaml
@mattamon mattamon requested a review from dvesh3 February 27, 2023 13:14
@mattamon mattamon merged commit 3b36cd4 into 11.x Feb 27, 2023
@mattamon mattamon deleted the 14260-refactor-sitemaps-and-redirects-bundle branch February 27, 2023 14:14
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.

[Task] Also move sitemaps and redirects to SEO Bundle

3 participants