-
Notifications
You must be signed in to change notification settings - Fork 132
MQE-2212: Fix invalid behaviour MAGENTO_BACKEND_BASE_URL #780
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
Conversation
f395707
to
7abcefe
Compare
*/ | ||
class MftfGlobals | ||
class UrlProvider |
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.
- This class is a util class that manipulates on the env vars. Let us not move it to top level directory structure.
- This class is mean to provide some global constants that can be reused across entire framewrok. Not necessary only for urls. I would rather keep it the same name.
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 think the reason why UrlProvider is used is to follow the same design pattern as Magento. Refer to UrlProvider
class in Magento. Not a bad idea to have Url builder class separated out from Global constants. But let me know if you still think if this needs to be refactored to the original.
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.
Which UrlProvider
class you refers to? The Provider
on root level file structure feels misleading.
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 UrlProvider
to Util folder, as per discussion
); | ||
} | ||
return UrlFormatter::format($baseUrl, $withTrailingSeparator); | ||
} catch (TestFrameworkException $e) { | ||
} |
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.
Let's keep the logic of caching the $baseUrl and $backendBaseUrl. I remember adding this to fix performance deterioration.
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.
$baseUrl changes based on $customArea specified in AdminFormExecutor and FrontendFormExecutor. Caching resulted in failures. How much of a degradation did you see?
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.
Do you have a build after the change? I remember there were degradation when I make these files change, but I don't remember exactly if it's due to code here.
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.
Had linked it in the ticket - https://m2build-ur.devops.magento.com/job/All-User-Requested-Tests/34863/
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 added file level caching for AdminFormExecutor
and FrontEndFormExecutor
. Will retrigger build after changes look good.
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.
if ($backendBaseUrl) { | ||
$backendBaseUrl = UrlFormatter::format($backendBaseUrl, $withTrailingSeparator); | ||
} | ||
return $backendBaseUrl; | ||
} |
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.
Do we still need this function? I mean will this be the same as getBaseUrl('admin')?
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.
getBaseUrl('admin') calls this function in the switch case.
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.
Does it need to be public
? I mean if the entry point is getBaseUrl('admin')
? It seems redundant.
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.
good catch, changed it to private
117006e
to
17898ad
Compare
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.
- The
Provider
directory is unnecessary. What other providers you expected to belong here? - The
baseUrl
andadminBaseUrl
are two global constants that will not change during test. I don't think we should change to calculate them every time based onarea
. Let's keep the original ideas to keep them cached as private static variables and accessible through getter functions of getBaseUrl() and getBackendBaseUrl() across entire framework. No need to store them separately inAdminFormExectuor
,FrontendFormExecutor
and WebApi Executor related classes. The changes should be much simplified. Let me know if you need more details.
Thanks for the comments, AC of this ticket was to port #547 in 3.0. If the AC is not acceptable, it's best to create a new ticket and plan it. What you think? |
I think I understood what you meant by simpilifying url generation. Updated to code to only pull in relevant pieces from Lukasz's PR just enough to fix the problem. Build: https://m2build-ur.devops.magento.com/job/All-User-Requested-Tests/35136/ |
Changes are simplified and Look good. Make sure to run UR build before merge. |
Build looks good - https://m2build-ur.devops.magento.com/job/All-User-Requested-Tests/35159/ |
Description
Ported changes from #547 to
develop
Fixed Issues (if relevant)
Contribution checklist