[RFC|Rejected] Revert all of ajax in 3.1

[Oleg]

The ajax code that was committed was never properly specified, designed or documented. Some documentation was written post-factum. This documentation is incomplete. It is partially in the code and partially in the wiki, which means inline documentation is even more incomplete. As the code changes the documentation that does exist becomes more out of sync with what the code does.

Case in point: https://github.com/phpbb/phpbb3/commit/ ... 53e59dd3b9. This change supposedly "fixes" an issue with ajax. It has no documentation delta. Was something broken? Does it remain broken? Was functionality that was claimed to work found not working? Was documentation written for that functionality? Was it changed to indicate that it is now not working?

The ajax that was added provides no significant benefit to users of phpbb. However dealing with the mess of its implementation is a regularly recurring feature of it. Specifically I'm talking about core.js. Therefore I propose nuking core.js out of the tree and doing a best-effort attempt to locate and either mark/note for future reuse/rewrite or delete added attributes to markup.

Then, should anyone want to add ajax to phpbb, I would require them to first specify what should be ajaxified, what the behavior of ajaxification should be, including error handling. This would be the spec. Next, any code written would have to come with inline documentation of everything. No anonymous functions are to be allowed. Each function would be required to have a complete docblock explaining what it does. Exceptions may be granted on a case-by-case basis but should not be relied upon. Finally, instead of working on 10 different features at the same time, each distinct feature should be implemented sequentially such that it can be properly reviewed and have nonzero quality.

[callumacrae]

I developed some AJAX stuff. You got *beep* off at the size of the PR. I stopped developing.

PR was merged.

It's clearly just not complete. With a little bit of work (I'll have some time over the summer, I'm guessing by the way that you posted this RFC that you're willing to do some stuff too), and I can do everything else.

You have no one to blame but yourself here.

[Arty]

No... just no. If you'll revert this, I doubt anyone will be bothered to redo it all.

Revert should never be used. Yet you seem to be happy to do it every time large patch is merged that was available on GitHub for few months and you didn't bother to review yourself. Do you want to redo it all yourself? Then go ahead, revert it. I doubt there are people motivated enough to redo all work over and over again because you couldn't be arsed to review it properly first time.

Better fix problems with small pull requests instead of reverting huge patch.

+1 on what callumacrae said.

[Erik Frèrejean]

I can't say that I'm a fan of the current implementation (or for what it is worth the whole AJAX thing at all), and indeed at the documentation side it also lacks a lot. But it has been merged so it

1. is up to standard
2. never got reviewed properly

in the first case then the standards should be reviewed/adjusted and the AJAX code/documentation be updated accordingly. The latter case is worse, if this got merged without proper review then what is the worth of the whole review process? Patches get stuck as PR for months and apparently they at some point get merged pretty much blind. In this case there is nobody to blame but the person that merged the "patch".

Simply nuking merged code because nobody could be bothered enough to review it properly, but still hit the merge button is a big -1.
The AJAX code and documentation should be improved where it can be improved but reverting it is a no-go unless someone has a different implementation ready that can be merged straight in as a replacement (I doubt that though).

[DavidIQ]

While it was not fun blindly guessing what things are doing and having a bit of a hard time while working on some code, I don't think the answer at this point would be reverting it all. Better or more clear documentation would greatly improve this.

There are quite a few shortcomings to the current implementation, one of them being the overuse of $.ajax for which I spent an hour or two last night trying to figure out why changing text on the page was taking a few seconds to change when there was no server call at all. Another being that the modal windows are just...horrendous and sometimes seem pointless. For instance when one clicks on the "Mark forum read" link you get the modal but the modal provides another link and also makes the entire page refresh instead of just marking the forum read by changing style attributes.

I think this needs a bit of massaging and documentation is all.

[imkingdavid]

If you want to revert this because it lacks the proper documentation of what it does, then you might as well just scrap the majority of the phpBB codebase, which also lacks proper documentation, and rewrite it from scratch (that's where 4.0 comes in).

If I remember correctly, I was the one who merged the AJAX PR, but only after testing it myself, reviewing it as best I could (I am not a javascript whiz, so I admit that I may not have been the right person to end up merging it), and being told by multiple people that it was ready for merge. Did I jump the gun? Perhaps. Is the answer to remove and scrap all of the work that has been put into it and start from scratch? I don't think so.

Whoever wrote it (I believe the vast majority was done by igorw and callumacrae) should be responsible for:
1) Documenting the code that is currently merged, both in the code and in the wiki, in a way that is
2) Review everything that has been merged to make sure it is all done properly and in an efficient manner
3) Make a list of everything that has been AJAX-ified, as well as the other things that should be AJAX-ified as well (this should be done within an RFC for discussion purposes)
4) Once the basic framework is complete, only work on one thing at a time, as Oleg said, so that it can be properly reviewed and tested without a bunch of other stuff getting in the way
5) Continue documenting as it progresses

[tbackoff]

I have to agree with Oleg here. The current implementation is of little use to our users (I was never a fan of ajax in phpBB in the first place, but that's my problem, not the community's ), not to mention it's still broken. However, if we are going to ajax, it needs to be throughout the entire core, not bits and parts. For example, why are we using trigger_error in some parts, but ajax in others? This makes no sense. Choose one, the other, or none at all.

I say rip it out and go back to the old way.

[callumacrae]

4) Once the basic framework is complete, only work on one thing at a time, as Oleg said, so that it can be properly reviewed and tested without a bunch of other stuff getting in the way

The trouble with that is that pull requests seem to take so long to get looked at - if I go through everything one at a time, it'll take months.

[MichaelC]

The code may have not been up to scratch, but it can't be worse than some of the code in phpBB. Why don't we just revert all that too?

Why does it need to be removed to do documentation? It can be documented by anyone at any time. Lack of documentation should not be a reason to remove a feature.

At the moment 3.1 is a very backed/admin heavy change. There aren't that many large frontend changes for end-users (there are some, just not alot), AJAX is one of the nicer ones.

I have to agree with Oleg here. The current implementation is of little use to our users (I was never a fan of ajax in phpBB in the first place, but that's my problem, not the community's ), not to mention it's still broken. However, if we are going to ajax, it needs to be throughout the entire core, not bits and parts. For example, why are we using trigger_error in some parts, but ajax in others? This makes no sense. Choose one, the other, or none at all.

I say rip it out and go back to the old way.

What I believe your referring to (quick mod, if not disregard this) is not still broken, it was fixed however Area51 is only updated to the latest 3.1 copy every now and then.

I agree AJAX is not the greatest code and could have used a little more reviewing; I also agree that the front-end could use a few changes. However I agree with Arty that you should have commented in the PR for the bits you thought should have been changed. Then we would not have the problem of mis-communication and imkingdavid thinking it was ready (I don't think it was his fault personally, I think he was just not fully informed due to over-use of IRC)

While I disagree against the reverting I think this does bring up a few valid points for future:
1) Over-use of IRC
While IRC is a great tool, not everyone reads absolutely everything in that channel. While it is great for discussion, it should always have a log posted or a summary posted in the RFC/PR/Ticket.
2) Too many discussion mediums
Partly linked to the previous comment. At the moment things are discussed in RFCs, PRs, IRC and Tickets. There should be a set of guidelines saying what gets discussed in the RFC, what gets discussed in tickets, what gets discussed in the PR. I'd suggest general implementation until a PR exists should be in the RFC. Nothing in the ticket. Once a PR is created all technical discussion should be in the PR, inline where possible. IRC should always be sumarised in the RFC or PR as appropriate. Obviously none of this applies to bug fixes as they should have all discussion in tickets until a PR exists, when all discussion should then be discussed there.
3) Multiple developers agreement
Just throwing this out there, perhaps say that for all new features it needs the agreement of 2 developers who have not committed to the PR before being merged after all changes (so not 1 developer agreeing, then a commit fixing something, then another developer agreeing). This means that if something slips past it is more likely to be caught and will prevent situations where a developer might not be aware of some facts regarding a feature.
4) One thing at a time
Don't try and implement every AJAX feature at once. Do it 1 ticket, 1 RFC and 1 PR for every new feature. Same thing applies to coding guidelines changes. Up until recently they were all part of 1 RFC, 1 ticket and lots of PRs. AJAX had numerous features in 1 RFC, lots of tickets and 1 PR.
5) Docblocks and Wiki Documentation
Every new feature should be documented for both the back and front-end. Docblocks should be added whenever possible
6) Functional or Unit Tests for everything
All bug fixes that fix things that can be tested (e.g. not typos) should have a test added. New features should have thorough unit/functional tests. Improvements should have tests when possible. If functional tests existed for this then it would have been caught before being merged. I'm not sure if functional tests currently support AJAX/JS but if not then we should possibly look at a way to incorporate this.

[callumacrae]

Nope, functional tests don't currently support JS. Another set of tests, using a client-side library, would need to be done, and it would have to be ran on the client side.