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

Fix https base tag generation. #6270

Closed
wants to merge 2 commits into from

Conversation

GregoryRusakov
Copy link

Fixed https base tag generation.
Discussion is here:
#4961 (comment)

Fixed https base tag generation.
Discussion is here:
joomla#4961 (comment)
@@ -73,7 +73,7 @@ public function fetchHead($document)

if (!empty($base))
{
$buffer .= $tab . '<base href="' . $document->getBase() . '" />' . $lnEnd;
$buffer .= $tab . '<base href="' .Juri::base(). '" />' . $lnEnd;
Copy link
Contributor

Choose a reason for hiding this comment

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

please add spaces like href="' . Juri::base() . '"

spaces added
@joomla-cms-bot joomla-cms-bot changed the title Update head.php Fix https base tag generation. Mar 7, 2015
@Hackwar
Copy link
Member

Hackwar commented Mar 8, 2015

Reading the code, it seems as if the base is set wrong somewhere. If you don't use getBase(), we can't influence this from for example a plugin. So why are you not fixing the place where the base in JDocument is set to a false value and instead replace the call with the static JURI::base()?

@ghost
Copy link

ghost commented Mar 16, 2015

I think the issue might be in libraries/cms/application/site.php
Which has:

if ($router->getMode() == JROUTER_MODE_SEF)
{
    $document->setBase(htmlspecialchars(JUri::current()));
}

Shouldn't that be?

if ($router->getMode() == JROUTER_MODE_SEF)
{
    $document->setBase(htmlspecialchars(JUri::base()));
}

@Sulpher
Copy link

Sulpher commented Sep 1, 2015

Peter provided solution, but it wasn't included in Joomla 3.4.3. Please add this patch in next Joomla version. Thank you.

@mbabker
Copy link
Contributor

mbabker commented Sep 1, 2015

If someone opens a pull request with his suggested change and it gets tested then it can be included in a future release. Until then, there isn't much that can be done. http://magazine.joomla.org/issues/issue-sept-2015/item/2837-did-they-fix-it-yet offers some good insight on how you can help ensure issues that you are having with the core software get resolved 😄

@Sulpher
Copy link

Sulpher commented Sep 1, 2015

Michael, thanks for the link. Sure, it's important to test all patches.
Well, I've tested this patch on various sites with SSL and Varnish - it works. Since the status of this fix is marked as checked I thought it's already revised.

@mbabker
Copy link
Contributor

mbabker commented Sep 1, 2015

The checks block just above the comment box here shows the status of our automated test suites, mainly to make sure that unit tests and codestyle checks continue to pass. Unless a pull request has the RTC label or has already been merged, then it still needs human review and testing.

@Sulpher
Copy link

Sulpher commented Sep 1, 2015

Ok, thanks for explanation. How many reviews are usually required to accept the patch?

GregoryRusakov added a commit to GregoryRusakov/joomla-cms that referenced this pull request Sep 1, 2015
Fixed https base tag generation.
Discussion is here:
joomla#6270
@Bakual
Copy link
Contributor

Bakual commented Sep 2, 2015

Usually two.

@b2z
Copy link
Member

b2z commented Sep 12, 2015

@GregoryRusakov can you update your PR to @nonumber solution? Seems that it is the right one.

@wojsmol
Copy link
Contributor

wojsmol commented Sep 17, 2015

@b2z I sent PR #7902 which implements solution suggested by @nonumber. @GregoryRusakov please test mentioned PR.

@zero-24
Copy link
Contributor

zero-24 commented Sep 17, 2015

Thanks @wojsmol so I'm closing here in favor of #7902 thanks

@zero-24 zero-24 closed this Sep 17, 2015
@wojsmol
Copy link
Contributor

wojsmol commented Sep 25, 2015

@Sulpher please test PR.#7902

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.

None yet

10 participants