Skip to content

Conversation

@karakayasemi
Copy link
Contributor

@karakayasemi karakayasemi commented Jul 28, 2017

Description

Dispatch event from possible 404 error locations.

Related Issue

owncloud-archive/security#3

Motivation and Context

The goal is to be able to detect 404 errors

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@DeepDiver1975 @Peter-Prochaska please review.

@DeepDiver1975 DeepDiver1975 self-requested a review July 31, 2017 08:27
@DeepDiver1975 DeepDiver1975 added this to the development milestone Jul 31, 2017
@DeepDiver1975 DeepDiver1975 changed the title [WIP] dispatch event for 404 errors dispatch event for 404 errors Jul 31, 2017
Copy link
Member

Choose a reason for hiding this comment

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

10.0.3

@karakayasemi
Copy link
Contributor Author

@DeepDiver1975 the version is corrected.

@DeepDiver1975 DeepDiver1975 requested a review from PVince81 July 31, 2017 09:27
@DeepDiver1975
Copy link
Member

@DeepDiver1975 the version is corrected.

thx - I'd like to get a second voice - @PVince81 mind having a look? THX

@PVince81
Copy link
Contributor

I guess GenericEvent would be too generic ?

If we're sending events for 404, should we also send some for 403, etc ? (if yes, separately)

@karakayasemi
Copy link
Contributor Author

karakayasemi commented Jul 31, 2017

I can rename ResourceNotFoundEvent class name as HttpEvents and we can dispatch all the events related to http by extending this class. Similar to following: https://github.com/owncloud/core/blob/master/lib/public/Comments/CommentsEvent.php . @PVince81 is it okay for you?

@PVince81
Copy link
Contributor

@karakayasemi renaming sounds good

@karakayasemi
Copy link
Contributor Author

@DeepDiver1975 @PVince81 please review. Thanks

Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍 from me

@PVince81 PVince81 merged commit 5cad1d6 into owncloud:master Aug 1, 2017
@DeepDiver1975
Copy link
Member

backport ?

@karakayasemi
Copy link
Contributor Author

Currently, only Security app will use this event. Security app min version probably will be 10.1, since #28206's milestone 10.1 and it was not backported. If we are planning to release security app for 9.1, we need to backport #28206 also, else IMHO no need to backport.

@phil-davis
Copy link
Contributor

This is not in stable10
@karakayasemi @PVince81 @DeepDiver1975
Backport to stable10 branch?

@karakayasemi
Copy link
Contributor Author

As far as I know, there is nothing which using this event currently, it was added for Security app, but nothing implemented yet. I am not sure that we want to keep it.

@PVince81
Copy link
Contributor

@karakayasemi I suggest reverting from master then as that would qualify as "API addition that we need to maintain and keep stable in the future" if we let it in stable10

@karakayasemi
Copy link
Contributor Author

karakayasemi commented Mar 19, 2019

revert PR in here #34835

@lock lock bot locked as resolved and limited conversation to collaborators Mar 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants