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

Add RewriteController to allow easy handling of historic rewrites #37

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

nhp
Copy link
Collaborator

@nhp nhp commented Jun 11, 2019

Added a new controller to be able to access the old rewrites vsf-api knows nothing about. It simply makes a DB call to core_url_rewrite, filtered by request_path and returns only the new final target.

Usage

Simply called by /vsbridge/rewrite/target?request_path=old_url this returns {"code":200,"result":"target_path"}

As we agreed with Łukasz Romanowicz on slack we should bring this functionality to the main magento1 trunk.

Copy link

@mkucmus mkucmus left a comment

Choose a reason for hiding this comment

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

Works fine. Can be merged @pkarw :). I will replace 500 with 404 if no route found in another issue.

Copy link

@sandermangel sandermangel left a comment

Choose a reason for hiding this comment

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

Hi Nils, thank you for the PR! I appreciate the new feature.
Left a tiny request to improve performance.


$reader = Mage::getSingleton('core/resource')->getConnection('core_read');
$select = $reader->select()
->from('core_url_rewrite', ['target_path'])->where('request_path = ?', $requestPath);

Choose a reason for hiding this comment

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

Please add a LIMIT 1 to the query to improve performance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I honestly thought that ->fetchOne() would do exactly like that but you get surprised by magento/Zend/Varien internals every now and then:) Limiting beforehand now introduced.

Choose a reason for hiding this comment

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

I know, it really sucks :/
Thanks!

nhp added 3 commits July 3, 2019 11:08
Though using fetchOne() returns the expcted result it is internally
limiting the result after querying it in full so this could lead to
performance impact on bigger cataloges to prevent this we added the
limitation directly to the sql query.
As core_url_rewrite stores rewrites based on store_id and request_path
we need both to identify a correct target. Added lookup of default
store_id=0 to the request to handle unspecific redirects at all. Order
by is needed to ensure, if default and store specific rewrites are found,
the default one is skipped and the more specific is used.
Added more specific http-status codes for different types of possible
results as 500 was not really the correct one to use.
@nhp
Copy link
Collaborator Author

nhp commented Jul 3, 2019

Also added store_id handling as this could also be an issue and we added more specific result-status codes to reflect on what really was the problem instead of only returning 200 and especially 500.

ResuBaka added 2 commits July 3, 2019 15:32
Improved store_id values in sql where
$select = $reader->select()
->from('core_url_rewrite', ['target_path'])
->where('request_path = ?', $requestPath)
->where('store_id IN (?)', [Mage_Core_Model_App::ADMIN_STORE_ID, (int)Mage::app()->getStore()->getId()])

Choose a reason for hiding this comment

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

I personally avoid using the globally set store id, had some issues in the past with having the wrong store.
might we require it to be set in the URL as param?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here i think we should have both, the Mage::app()->getStore()->getId() as a fallback when no storeId is set in the request?

Like for example:

$storeId = $this->getRequest()->getParam('storeId');
if (empty($storeId)) {
    $storeId = (int)Mage::app()->getStore()->getId();
}

What do you think about that? @sandermangel

@nhp
Copy link
Collaborator Author

nhp commented Jul 10, 2019

Do we still have open issues here?

@nhp
Copy link
Collaborator Author

nhp commented Aug 21, 2019

Any update on this one?

@ResuBaka
Copy link
Collaborator

Here I have found a small Problem. I will Update the PR in the next days.

@sandermangel
Copy link

@ResuBaka hi! any update on the PR changes?

@ResuBaka
Copy link
Collaborator

I will update in the next days.

The problem is that I need to send the request URLs html encoded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants