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

Added new interface FilterInterfaceWithArguments #3217

Closed
wants to merge 3 commits into from
Closed

Added new interface FilterInterfaceWithArguments #3217

wants to merge 3 commits into from

Conversation

tangix
Copy link
Contributor

@tangix tangix commented Jul 4, 2020

Description
Fixing #3216 by implementing a new interface to avoid breaking already implemented Filters.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@tangix
Copy link
Contributor Author

tangix commented Jul 4, 2020

All tests pass locally, so I am not sure what I'm seeing...

@michalsn
Copy link
Member

@tangix Can you pull the latest changes from the dev branch and add Services::reset() to the FiltersWithArgumentsTest::setUp() just after the call to the parent::setUp()?

@tangix
Copy link
Contributor Author

tangix commented Jul 13, 2020

Done.

@michalsn
Copy link
Member

@lonnieezell @MGatner

We probably need to update the user guide for this, but besides that - are you good with these changes?

@tangix
Copy link
Contributor Author

tangix commented Jul 13, 2020

@lonnieezell @MGatner

We probably need to update the user guide for this, but besides that - are you good with these changes?

Also, please consider if there is a better way to do this without breaking backward compatibility.

@lonnieezell
Copy link
Member

So basically all we're doing is creating a new interface that accepts $args, correct? In my mind, this is a bug since it is expected to be working currently. So all we need to do is add the argument to the interface and method. And maybe add some better docs, right?

While I realize this is technically a BC break, it won't break any existing code. And should be working actually since Myth:Auth uses those filters, though I haven't tried it in a while.

@tangix
Copy link
Contributor Author

tangix commented Jul 14, 2020

@lonnieezell
Yeah, I was torn between changing the current interface (breaking code) or adding a new interface (possibly cluttering down the framework)...

@lonnieezell
Copy link
Member

@tangix And I totally respect that you were trying to avoid the BC break here and really appreciate it! I just think this qualifies as a bug so we can fix it the right way.

@tangix
Copy link
Contributor Author

tangix commented Jul 14, 2020

@tangix And I totally respect that you were trying to avoid the BC break here and really appreciate it! I just think this qualifies as a bug so we can fix it the right way.

I can live with that and I think that would be the correct way forward to avoid another interface and added complexity to CI4.

@tangix
Copy link
Contributor Author

tangix commented Jul 15, 2020

@lonnieezell
I see 4.0.4 is around the corner but with no fix for this issue. Do you want me to create a new PR based on develop branch, change the FilterInterface (introduce a BC breaking fix) and also check over the docs?

@MGatner
Copy link
Member

MGatner commented Jul 15, 2020

@tangix the goal for 4.0.4 is within the next 24 hours. If you can get this reworked and submitted today I think it would be a great addition given that developers using it will require manual intervention, and a point release is a great time for that.

@tangix
Copy link
Contributor Author

tangix commented Jul 15, 2020

OK, created #3316 fixing the actual bug instead of adding a new interface.

@tangix tangix closed this Jul 15, 2020
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

5 participants