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

Optimizing mod_login helper #6034

Closed
wants to merge 3 commits into
base: staging
from

Conversation

Projects
None yet
10 participants
@Hackwar
Member

Hackwar commented Feb 9, 2015

We already have loaded all menu items. We don't need to look up an existing menu item again. If JMenu does not return this item, it either does not exist or it is unpublished or not accessible for the user. The router also makes sure that all parameters of a menu item are correctly stripped. There is no need to differentiate between SEF and non-SEF URLs.

How to test

  • See current behavior of mod_login
  • Apply patch
  • Notice that performance improves and nothing else changes.
@810

This comment has been minimized.

Contributor

810 commented Feb 9, 2015

Fatal error: Class 'ModLoginHelper' not found in /home//domains//public_html/modules/mod_login/mod_login.php on line 17
j3.4 b2

Mhh other patches same issues, maybe my test site is broken.

@brianteeman

This comment has been minimized.

Contributor

brianteeman commented Feb 9, 2015

@810 what were you doing to get that error. I couldnt replicate it myself


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/6034.

@810

This comment has been minimized.

Contributor

810 commented Feb 9, 2015

@brianteeman i applied patch with patchtester, on a ssl website.

@test
With patch:
with
Without patch:
without

@brianteeman

This comment has been minimized.

Contributor

brianteeman commented Feb 9, 2015

Strange as I cannot get that error message (perhaps because I am not on an
SSL site?)

On 9 February 2015 at 23:37, Jelle Kok notifications@github.com wrote:

@brianteeman https://github.com/brianteeman i applied patch with
patchtester, on a ssl website.

@test https://github.com/test
With patch:
[image: with]
https://cloud.githubusercontent.com/assets/876623/6118345/dcabcf86-b0bc-11e4-8d0d-5c675272b35e.jpg
Without patch:
[image: without]
https://cloud.githubusercontent.com/assets/876623/6118354/eeb68bbc-b0bc-11e4-991e-28baf24f7f33.jpg


Reply to this email directly or view it on GitHub
#6034 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

Hackwar added some commits Feb 10, 2015

@ceus1984

This comment has been minimized.

ceus1984 commented Mar 14, 2015

Indeed, nothing changes but the performance does not get much better either

@msdevcon

This comment has been minimized.

msdevcon commented Mar 14, 2015

@test
Still works fine, but no big difference.

@Hackwar

This comment has been minimized.

Member

Hackwar commented Mar 14, 2015

It is 50 lines of code less. Yes, the performance improvement is negligible.

@coolcat-creations

This comment has been minimized.

Contributor

coolcat-creations commented Mar 14, 2015

Works normal :-)


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/6034.

@waader

This comment has been minimized.

Contributor

waader commented Mar 14, 2015

@test works for me. Thanks!

@zero-24

This comment has been minimized.

Contributor

zero-24 commented Mar 14, 2015

RTC thanks for testing!


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/6034.

@brianteeman brianteeman added the RTC label Mar 14, 2015

@phproberto phproberto added this to the Joomla! 3.4.1 milestone Mar 15, 2015

@phproberto

This comment has been minimized.

Contributor

phproberto commented Mar 15, 2015

Merged. Thanks!

@zero-24 zero-24 removed the RTC label Oct 14, 2015

@Hackwar Hackwar deleted the Hackwar:patch-28 branch Jan 6, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment