Skip to content

Conversation

@andrepereiradasilva
Copy link
Contributor

Summary of Changes

This PR does for weblinks what was already done and merged for the core.
Replace existing 404 JError for a 403 php exception (JAccessExceptionNotallowed) when the user does not have access to "Access Administration Interface" (core.manage).

See also joomla/joomla-cms#11608

Testing Instructions

Code review.

@zero-24
Copy link
Contributor

zero-24 commented Aug 19, 2016

👍

@mbabker
Copy link
Contributor

mbabker commented Aug 19, 2016

There should be a manifest script added to the package blocking installs on
older Joomla versions if this is accepted. Otherwise you'll get class not
found errors.

On Friday, August 19, 2016, andrepereiradasilva [email protected]
wrote:

Summary of Changes

This PR does for weblinks what was already done and merged for the core.
Replace existing 404 JError for a 403 php exception
(JAccessExceptionNotallowed) when the user does not have access to "Access
Administration Interface" (core.manage).

See also joomla/joomla-cms#11608
joomla/joomla-cms#11608
Testing Instructions

Code review.

You can view, comment on, or merge this pull request online at:

#262
Commit Summary

  • Add an exception when not allowed

File Changes

Patch Links:


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#262, or mute the
thread
https://github.com/notifications/unsubscribe-auth/AAWfodqt7FrAyg_euRPdcnlHf9RFpHiwks5qhjWQgaJpZM4Jo65Y
.

@andrepereiradasilva
Copy link
Contributor Author

There should be a manifest script added to the package blocking installs on older Joomla versions if this is accepted. Otherwise you'll get class not found errors.

i know and has about to write that here.
Yes, must be J! 3.6.3+ to get the correct message in the error or will get an error also but with an incorrect message JAccessExceptionNotallowed not found or something.

if (!JFactory::getUser()->authorise('core.manage', 'com_weblinks'))
{
return JError::raiseWarning(404, JText::_('JERROR_ALERTNOAUTHOR'));
throw new JAccessExceptionNotallowed(JText::_('JERROR_ALERTNOAUTHOR'), 403);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like the name of that exception. In English it reads like the exception is not allowed! But I suppose there isn't much to be done about it given the autoloader, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty much. In our namespaced code the Exception objects either get into
their own namespace or don't have this issue of fighting with a camel cased
autoloader but can enjoy the benefit of PSR-4 rules. I don't like the
names of the cache and database exceptions I added either but it's all the
same problem in the end.

On Friday, August 19, 2016, Chris Davenport [email protected]
wrote:

In src/administrator/components/com_weblinks/weblinks.php
#262 (comment)
:

@@ -12,7 +12,7 @@

if (!JFactory::getUser()->authorise('core.manage', 'com_weblinks'))
{

  • return JError::raiseWarning(404, JText::_('JERROR_ALERTNOAUTHOR'));
  • throw new JAccessExceptionNotallowed(JText::_('JERROR_ALERTNOAUTHOR'), 403);

I don't like the name of that exception. In English it reads like the
exception is not allowed! But I suppose there isn't much to be done about
it given the autoloader, right?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/joomla-extensions/weblinks/pull/262/files/cfa32e7ec46893a49d281460600711c8df8ec7d6#r75564612,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoQ1qh4gcBAC8W_OBJRSH6qdp5LU6ks5qhjr7gaJpZM4Jo65Y
.

@zero-24
Copy link
Contributor

zero-24 commented Sep 23, 2016

Can we move this forward? As we have the same exeption JAccessExceptionNotallowed in the core we can go with it?

@andrepereiradasilva
Copy link
Contributor Author

this can only be merged if the next weblinks version is only 3.6.3+

@yvesh
Copy link
Member

yvesh commented Oct 2, 2016

@andrepereiradasilva imo weblinks should only support latest stable release. But it's probably a PLT decision needed on that..

@chrisdavenport
Copy link
Contributor

@andrepereiradasilva Can you update this PR to block installs prior to 3.6.3? I can't think of any good reason to support earlier versions. Thanks.

@andrepereiradasilva
Copy link
Contributor Author

i think that should not be related to this PR, but the release itself

@yvesh yvesh merged commit 5178381 into joomla-extensions:master Feb 13, 2017
@zero-24 zero-24 added this to the 3.6.0 milestone Feb 13, 2017
@yvesh
Copy link
Member

yvesh commented Feb 13, 2017

Thank you @andrepereiradasilva, we will add a check that we only support latest 3.6 version.

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.

5 participants