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

[5.3][SEF] Remove parent_key from url in NoMenuRules #44460

Open
wants to merge 6 commits into
base: 5.3-dev
Choose a base branch
from

Conversation

heelc29
Copy link
Contributor

@heelc29 heelc29 commented Nov 14, 2024

Summary of Changes

the new preprocessrule (#43992) adds the parent key during build process but if no menu item exist for this component there is no redirect to sef url

This reverts commit #4f2a09ef42676f91647ebecdef27bf1e73117e17 and restore same behavior like in J5.2

Testing Instructions

Actual result BEFORE applying this Pull Request

no redirect to sef url

Expected result AFTER applying this Pull Request

redirect to NoMenuRules sef url: /index.php/component/contact/contact/{contact.alias}

Link to documentations

Please select:

  • No documentation changes for docs.joomla.org needed
  • No documentation changes for manual.joomla.org needed

@Hackwar
Copy link
Member

Hackwar commented Nov 14, 2024

Unfortunately this isn't that easy. Simply removing this at this point is a break in b/c for the routing. Yes, the query parameter there has been wrong and it has been wrong for years, also already in Joomla 3, but simply removing it means lots of errors in the google search console, so we should carefully consider how to handle this. I've been thinking about this for a long time already and don't have a satisfying solution for this. The problem is that we also need the information about the category for URLs without IDs. The way the PR is now, it can't be merged unfortunately.

@richard67
Copy link
Member

@Hackwar Maybe I misunderstand something, or I lack some knowledge, but how can this PR be a b/c break or make problems in Google search if this PR reverts a change which was made in 5.3-dev to what we have in 5.2?

@richard67
Copy link
Member

@heelc29 I allowed myself to update your branch to the latest changes in 5.3-dev and have resolved conflicts.

Now the system tests are failing at the last test:

1) Test in frontend that the contact site router
       can process contact with legacy routing:
     AssertionError:
       expected 'http://localhost/cmysql/index.php/component/contact/contact/38-test-contact-router-test-contact-router'
       to match /\/index.php\/component\/contact\/contact\/38-test-contact-router$/
      at __webpack_modules__</</overrideChaiAsserts/ (http://localhost/__cypress/runner/cypress_runner.js:139310:25)
      at overwritingMethodWrapper (http://localhost/__cypress/runner/cypress_runner.js:79011:33)
      at  (webpack://joomla/./tests/System/integration/site/components/com_contact/Router.cy.js:172:44)

It seems the "test-contact-router" is appended 2 times in the URL.

@Hackwar
Copy link
Member

Hackwar commented Nov 23, 2024

@richard67 I introduced a new rule which fixed URLs which didn't contain the necessary information, here: It adds the catid. This happens if you don't provide a catid when generating the URL. But by default all URLs generated by the core already have the catid and this PR all of a sudden removes those catids again. While this works okay when using IDs, URLs with IDs disabled will not be able to discover the right article.

@richard67
Copy link
Member

@richard67 I introduced a new rule which fixed URLs which didn't contain the necessary information, here: It adds the catid. This happens if you don't provide a catid when generating the URL. But by default all URLs generated by the core already have the catid and this PR all of a sudden removes those catids again. While this works okay when using IDs, URLs with IDs disabled will not be able to discover the right article.

@Hackwar Could you advise then how else we could get rid of the workarounds in the unit tests?

@Hackwar
Copy link
Member

Hackwar commented Nov 23, 2024

The easy solution would be to simply use the catid in the URL as well.

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.

4 participants