feat: add endpoint for setting MW DB version#1010
Conversation
deer-wmde
changed the title
outdooracorn
left a comment
outdooracorn
left a comment
There was a problem hiding this comment.
Submitting comments I had so far before going for lunch :)
| * this map of mediawikiVersion -> dbVersion lives. | ||
| */ | ||
|
|
||
| namespace App\Helper; |
There was a problem hiding this comment.
[non-blocking] I'm now wondering if MediaWikiHostResolver should be a helper as well, rather than a service?
There was a problem hiding this comment.
I don't know - I think it was something I suggested back then, but I wouldn't change it again now since we tested the implementation already in all the Jobs etc.
Also as a side note I thought it was quite nice to see this Service pattern getting implemented and how its use can play out with Dependency Injection, so it could be a good example for possible future Service classes
Co-authored-by: Ollie <43674967+outdooracorn@users.noreply.github.com>
|
trying to summarize what I wondered about API Design here: comparing the patterns used in this PR to @rosalieper I'm unsure which I like more: https://github.com/wbstack/api/pull/1009/files
Overall I think as a team we never fostered much design choices here, so currently we just see lots of different approaches in different parts of the codebase. More compartmentalized code might be the better choice to deal with these circumstances. But we may also miss chances of cleanups we could do along the way. Any thoughts around this from anyone? |
|
Thanks for writing your thoughts down, Dena. I think that we should start with a WET implementation of the
EDIT: Just to be clear, I really want us to improve the code while we are touching it and have the context loaded into our brains. I just think that will be easier to do in separate PRs. :) |
you are definitely right; in the absence of an obvious pattern to follow I think both are acceptable and I'd not want to block this PR on how it should look. I think you are right that we should probably try and set some more careful design decisions though. My personal preference is for smaller single classes with one endpoint and many methods (GET, POST) etc. in that file |
successor of #1010 Adds the backend endpoint `/backend/setWikiDbVersion` as defined in https://phabricator.wikimedia.org/T410394
separated from #1010 - adds input validation - adds unit test - makes wiki retrieving safer - removes some special dev handling that is to my knowledge not needed anymore - IMO the routing in backend.php here also would need a cleanup, but that would break client use, so.. meh.
successor of #1010 Adds the backend endpoint `/backend/setWikiDbVersion` as defined in https://phabricator.wikimedia.org/T410394
separated from #1010 - adds input validation - adds unit test - makes wiki retrieving safer - removes some special dev handling that is to my knowledge not needed anymore - IMO the routing in backend.php here also would need a cleanup, but that would break client use, so.. meh.
3 participants
https://phabricator.wikimedia.org/T410394
Bug:T410394