Page MenuHomePhabricator

Github-style Remarkup previews on a checked-in resource
Closed, WontfixPublic

Assigned To
Authored By
rmnoon
Jul 24 2014, 4:19 AM
Tokens
"Like" token, awarded by marcio-ao."Like" token, awarded by 20after4."Like" token, awarded by kakaroto."Doubloon" token, awarded by spage."Like" token, awarded by zmphabricator."Love" token, awarded by ideasman42.

Description

It'd be neat if Phabricator had github-style previews for checked-in docs (in markdown, remarkup, or really anything rich).

For example:
https://secure.phabricator.com/diffusion/P/browse/master/README.md

should have the utility of something like this:
https://github.com/less/less-docs/blob/master/README.md

This has a couple of wins:

  1. Simple documentation can easily be reviewed with the rest of code (on content and formatting!)
  2. Documentation is much more likely to be consistent on a commit-by-commit basis with the systems it's documenting.
  3. It's easy to make the world's simplest wiki.

We could just add a param to the URL to specify pretty formatting. Also, integrating Github-flavored markdown would make life easier for those of us who work in both universes regularly. Happy to make a separate task for that.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
rmnoon added a subscriber: ddfisher.

I know @ddfisher looked into this a little bit recently, not 100 percent sure what his conclusions were as to its feasibility.

It looks like adding GFM or other markdown renderers has negative security implications, among other issues (see this discussion and T2849). We just want some sort of markdown renderer (i.e. remarkup) for checked-in files, which I think (without too much investigation) should be pretty straightforward to implement.

Yep, @ddfisher's assessment is accurate:

  • We can very reasonably add a browse/render/display mode. The easiest renderer is Remarkup, but down the road we could do things like GitHub's "preview 3d model" feature or render SVGs or .dot or whatever else, as use cases arise. (Ideally, it would be nice to make these renderers diff-aware so they have an opportunity to do a visual diff of resources like these. At least for markup, though, this seems likely to be quite complicated.)
  • T2849 discusses adding alternative renderers (GFM, "real" Markdown, etc.). Feasible, but we need to be confident we aren't introducing security issues, which makes them a lot of work.

Question might be dumb but what kind of security issues are you talking about? I just can't come up with any.

The security risk is that if a markdown implementation does not escape HTML correctly, an attacker with access to commit to any repository (or create a repository) can push a README file which renders with Javascript in it, then trick a user into viewing the page. After the user clicks the link, the Javascript can read their CSRF tokens and make requests which impersonate them.

I don't know what the policy of the phabricator devs is, but i would leverate existing libraries for that kind of functionalities. They should have cared enough about those questions.
Since i just recently found phabricator I'm not that much into source yet. I know it's a bit offtopic and it has been discussed on other places, but when you would use Composer for dependency management, it would make a lot of things much easier. ( D5819 )
As a library i would recommend Parsedown.

Sadly I haven't been able to find the autoloading module in phabricator yet. If someone would hint me where to find it I might dig into it and try to rewrite it to be PSR compilant (please see other PSR's aswell).

Our policy is not to trust third-party code by default. A lot of third-party code is written with lower security standards, or is expected to run in a different environment than Phabricator provides. Third party code may also have performance issues or fail in ways that make it difficult to debug, so even if we don't make security changes we may need to add instrumentation, make performance-related changes, or change error handling. You can find more technical discussion of Remarkup vs other engines in T2849.

Generally, we're not interested in adding dependencies unless they provide a huge amount of immediate value. Dependencies have a large cost over time.

Can you explain the concrete problems we face today that Composer and PSR compliance would help solve?

IMHO 3rd party libraries aren't that bad (at least the "big ones"). E.g. if you look at Parsedown being nearly 1.4k times faved and a huge active bugtracker I don't see how it would be better to implement such functionality by yourself. Of course you shouldn't just use whatever library there is available but there are some high quality ones out there.

Problem i see with not being compilant with PSR and not using Composer is taking away the possibility to use those libraries. Also I've looked at your installation instructions (set up a Vagrant box for evaluation of Phabricator in a sandbox) and realised that for installation you need to git clone multiple repositories. Of course you could define the dependencies as submodules in your main Phabricator repo but using Composer is just more "sate-of-the-art" I'm afraid.

PS: I don't want to offense you and the other dev's in any way. I'm considering participating in the Phabricator project and think it's some great piece of software already!

taking away the possibility to use those libraries

We can and do use third-party libraries by embedding them in the repository in externals/. In general we must do this, because we modify the code for almost all dependencies (to improve security, add instrumentation, address performance issues, fix bugs, improve error handling, etc).

In contrast, managing dependencies with Composer would make installing and updating Phabricator more complicated, because it would require installs to install and learn Composer. Almost all installs are already familiar with Git, but many do not use PHP and are not familiar with Composer.

So we think this wouldn't solve any problems we have today (managing dependencies isn't really a problem), would add new problems (users needing to install and learn Compower) and wouldn't let us do anything new (we can't use Composer to manage external dependencies because we modify most of them).

  1. How are you managing autoloading classes for external libraries? (source code refference please)
  2. Why aren't you sharing the improvements made via PR's in the repos? (Or are you?)
  3. Installing / updating Phabricator using Composer would be as simple as composer install or composer upadte as long as the required version in the composer.json would be @dev etc.

I'm also quite new to technologies like Composer or PSR compilant code since the technologies are rather new themselves. However I've experienced quite a steep and straight-forward learning curve which lead to great success and ease in my recent projects.

Edit: A further advantage of using Composer would be, that each module that is in Phabricator could be hosted in their own repository and therefore developed some sort of individually. It also would be easy to manage what modules the end-user actually want's to use in Phabricator by requireing them explicitly in the composer.json. For convenience there could be a meta-package for Phabricator, that already requires all available modules.
Since I'm still quite new to Phabricator's codebase I'm not sure, whether decoupling Phabricator's modules that strict would even be possible without huge effort, but I wanted to mention it in case it was possible.

How are you managing autoloading classes for external libraries? (source code refference please)

We don't currently use any libraries which require us to manage autoloading.

Why aren't you sharing the improvements made via PR's in the repos? (Or are you?)

We do. Some changes have been pulled upstream. Some upstreams are unresponsive. Some aren't interested in changes that are important to us. Some changes are specific to Phabricator infrastructure. Some changes would require a lot of time to try to upstream, etc.

Installing / updating Phabricator using Composer would be as simple as composer install or composer upadte as long as the required version in the composer.json would be @dev etc.

When we add a dependency (here, a dependency on Composer), we need to support it when things go wrong. It seems very unlikely to me that all our users will think installing Composer is easy and fun and not have any problems with it. None of us are familiar enough with Composer to support errors users encounter with it, so we'd have to become familiar first.

Edit: A further advantage of using Composer would be, that each module that is in Phabricator could be hosted in their own repository and therefore developed some sort of individually.

We make a lot of API changes right now that affect all applications, so this would slow us down a lot, because we'd have to make those changes in each module individually. See T5447 for some discussion of stability and third-party applications.

It also would be easy to manage what modules the end-user actually want's to use in Phabricator by requireing them explicitly in the composer.json. For convenience there could be a meta-package for Phabricator, that already requires all available modules.

Users can already manage this with a web UI, which is much easier for users (and less error prone) than editing a JSON file.

Thank you for your reply.

However I think some clearification is needed:
Composer is implemented in PHP itself and is installed via a single command (refference). Since it's got PHP under its own hood, it runs as long PHP is configured proberly which we can assume for current end-users.
Also Composer does nothing else then resolving defined dependencies (taking into account versions). This means, that we would define a core module (libphutil in this case) which has all the shared functionality that all modules need. Furthermore we would define an own package for each module that has the core package as dependency. If an end-user decides to just grab that individual module, Composer then would resolve the dependency to the core module and install it aswell assuring that both library versions are compatible.

We make a lot of API changes right now that affect all applications, so this would slow us down a lot, because we'd have to make those changes in each module individually. See T5447 for some discussion of stability and third-party applications.

This seems like a valid point to me. Workflow would change rather drasticly and ther would be more maintanance (with every minor release of a new module the depending versions might be modified to ensure that everything that depends on each other is in sync).

Still I'm convinced, that rearanging some core structure of Phabricator in order to perform the changes i described above would improve the code quality and usibility of phabricator itself. Moreover I think more potential developers like me would join the project and help developing it since Composer gained more and more importance to web development over the past 3 years.

Quick side note on PSR compliance: Well it's quite simple. Those standards where made from leading PHP software companies in order to create standards that allow interopability with 3rd party libraries and are wideley used already. Please refer to the PHP Framework Interop Group's website for further information.

I'm familiar with Composer, in the sense that I've used it in the past. I'm not familiar enough with it to support it. I don't know how to resolve common errors, or what those errors are.

Whenever we add a new feature or an external dependency, that change comes with a support cost. Users will not understand how to use the thing or why to use the thing or when to use the thing, or have trouble installing or configuring it, or want it to work differently or better, or it won't work in their environment, or be compatible with other things they do or use, or whatever else. This support cost is usually higher for complex features and dependencies, because there is more stuff that can go wrong. I anticipate that Composer would have a high support cost:

  • It is a large, complex tool.
  • Many users aren't very familiar with PHP or Composer.
  • I am not familiar with the inner workings of it, so I'd have to learn those at a minimum.
  • We'd need to write documentation for it, and help all existing installs migrate to it.

We have never added a dependency which just works right all the time for all users. This includes popular software like Git (for example, Git 1.7.1 has a bug with origins when cloning repositories that at least 5 users have hit), Mercurial (for example, Mercurial has a bug between 2.1 and 2.1.1 where hg pull returned the wrong exit code which several users hit), PHP (for example, some users have the FCGI SAPI in place of the CLI SAPI on the CLI), APC (for example, APC 3.1.14 is broken) and MySQL (lots of common configuration issues). A tool or library being popular does not mean it works correctly for everyone all the time. These are just examples off the top of my head -- there are many, many, many more. Users routinely make mistakes with PHP and Git and Mercurial, misconfigure them, misunderstand how to use them, have different versions or multiple different versions or generally broken systems. We have to support all of this.

So, based on all of the dependencies we've added so far, my experience is that adding dependencies always has a support cost and this cost is roughly proportional to the complexity of the dependency. It is certainly possible that Composer is an exception, but that seems very unlikely. Googling for Composer problems reveals plenty of people on, e.g., StackOverflow having issues with it. If we add a dependency on Composer, those issues will come to us.

The biggest thing holding Phabricator development back is how time consuming it is to support it. I woke up about three hours ago and have spent that entire time digging through the support queue (tasks, email, IRC, GitHub issues, etc.), including this ticket. I will probably spend another 30-60 minutes before I catch up. That's just triaging issues: I haven't actually fixed any of the stuff that came up yet. All in all, I'll probably spend 6-8 hours today on support. I do this pretty much every day.

I have a high degree of confidence that adding a Composer dependency would cost us hundreds of hours in support costs over the lifetime of the project, based on the complexity of Composer and my experience supporting similar dependencies. The benefits of adding support need to be extremely compelling to justify this kind of cost. I am not nearly as sure as you are that adding Composer would improve code quality or usability (I think it would decrease usability and be moot for code quality), and don't believe developers who will have a lifetime net positive impact to the project are likely to be turned away because we don't use Composer.

I'm also familiar with PSR. Likewise, I see no concrete benefits. We don't have problems interoperating with third-party code, and very few users have difficulty interoperating with us. We don't want to support inbound interoperation from users now anyway, for other reasons. We also support PHP 5.2.3, which does not have namespaces. We have a much more powerful static analysis and autoloading system, which PSR would not replace. Code depending on Phabricator (vs libphutil) can never use PSR to autoload, anyway. The costs to switch to PSR would be substantial.

First I think we're drifting away from the original ticket topic. Shall I open a new ticket or is it possible to kind of move this discussion to another ticket?

I think we are having different philosophies on software development. Sure it's also a question of who your end-users are, but for an open source project (other than closed source enterprise software) I would hardly ever fix bugs others have introduced in their library i'm using and instead use the latest stable version out there, create a ticket in their bugtracker to let them fix it and upgrade if the bug/issue is solved. Also missconfiguration shouldn't be an issue that should be adressed by trying to avoid missconfiguration but rather have good documentation and free support like in an IRC channel to address those kind of problems.

What you are saying (- and I understand that -) is, that you're trying to give the best support you can but if you leveraged more 3rd party libraries you'd have the experts there and could delegate the issues, that occure in their libraries to them, since they're the experts in what they are doing and you're the expert in what you are doing.

I don't want to force my oppinion on you in any way but I'm deeply convinced that leveraging other's and their expertise leads to better software for everyone should be the way to go.

To bring this a bit back on topic, I think adding a Remarkup-rendered view that was not diff aware (as a first pass) would be very useful at pretty low implementation cost.

@epriestley, I'm interested in implementing this. Do you have suggestions for where I should start or unexpected issues I might run into? With regards to UI, I'm currently envisioning a "View Rendered File" button in the diffusion file contents view next to "Open in Editor" and "View Raw File" - does that seem like the best place to put this?

@ddfisher: That sounds reasonable to me. I don't think there are any particular pitfalls to watch out for. We could make this the default view for recognized file extensions once it seems to work, too.

At some point we could maybe merge some of the view options into dropdowns, but we can easily do that later on.

I would also like to see the remarkup-rendered view for any .md file in the repo, not just the Readme. That would make add documents to the repo and discover them a lot easier.

chad triaged this task as Wishlist priority.Nov 1 2014, 4:06 PM
chad added a project: Remarkup.
avivey renamed this task from Github-style Markdown / Remarkup previews on a checked-in resource to Github-style Remarkup previews on a checked-in resource.Nov 11 2015, 9:07 PM
avivey added a project: Diffusion.
avivey moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Dec 27 2015, 3:13 AM

Tagging Wikimedia - This task would resolve a regression we experienced after having moved from Gerrit/Gitblit to Phabricator Diffusion as primary repo browser. Gitblit, similar to GitHub, rendered .md files as Markdown by default. (Remarkup would be fine, too.)

For example when linking to a change log:

https://gerrit.wikimedia.org/r/288124

https://phabricator.wikimedia.org/diffusion/GOJU/browse/master/History.md

avivey moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.May 17 2016, 12:59 AM
avivey moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Jul 12 2016, 9:55 PM

T11747 wants a more advanced version of this (reviewing diffs between rendered documents), although the specific document type it wants (IPython notebooks) is likely very complex to render.

@epriestley this doesn't seem hard to build for a fledgling designer/intern engineer.

  • Add a button in the file header for Remarkup or Text
  • Save a per user preference
  • Render it however they last selected

I think we could even default it to "fancy mode" without a setting, at least to start with. I'm not sure anyone will want to access the plain text mode with enough frequency that we need it to be sticky.

It's also potentially good if the plain-text mode picks up a ?render=plain sort of URL marker, so that sending someone that link renders in plaintext mode. Requests with a line number should also force plain text mode.

I don't think this should be specific to remarkup or one-per-filetype, though. For example, you might want to view SVG as an image (since it's an image), or view SVG as a DOM (since it's also an XML file). In this case, you'd have three different options: render as plain text, render as XML/DOM, render as SVG.

But, yes, this is pretty reasonable for someone without tons of Phabricator experience to build.

chad raised the priority of this task from Wishlist to Normal.
chad edited projects, added User Delight; removed Contributor Onboarding.

I'm going to probably re-design this page a little.

@Krinkle, @pablocarrillo I second your requests. Great suggestion. I have a large README.md that I would love to split into multiple documentation files, without losing all the formatting.

Oh hai Cura uses Phabricator? I have a Lulzbot at home!

In T5698#223992, @chad wrote:

Oh hai Cura uses Phabricator? I have a Lulzbot at home!

Absolutely! :)

https://code.alephobjects.com

chad removed chad as the assignee of this task.Aug 6 2017, 2:46 AM
epriestley claimed this task.

See followup in T13105.