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

Composer roadmap #1890

Closed
Rarst opened this Issue Jan 12, 2015 · 36 comments

Comments

Projects
None yet
4 participants
@Rarst
Contributor

Rarst commented Jan 12, 2015

Hi! I am your resident Composer menace.

I am working to make WP SEO have awesome Composer support. Because Composer is what keeps developers happy and managing dependencies reliable (audience cheers).

As well as hopefully simplifying development and build processes away from Git Submodules (audience moans).

The current state

  1. Present composer.json in main plugin, but broken due to submodule use
  2. Two submodules without Composer support

Decision points

Is it time for submodules to go?

Should the submodules be retained in parallel? Or should Composer become main and preferable mechanism with submodule structure deprecated?

How will we handle autoload?

The tricky part is to enable end result to work seamlessly with PHP 5.2 (audience moans).

While you can use Composer to build for PHP 5.2 target without issue, its autoload is inherently 5.3.

From my research so far I see two approaches:

  1. Ignore Composer autoload, rely on known dependency locations in tree to load files. Vendor directory can be discarded in build process. As part of the site stack the built in load should be ignored in favor of global Composer autoload.
  2. Use the intermediary classmap, generated by Composer. While that "magic" autoload in Composer requires 5.3, it also generates intermediary files which seem not too. One of those can provide classmap, mapping classes to their filesystem locations. This file can be used in internal autoloader, over hardcoding the locations.
  3. composer-php52 was brought to my attention as an option for autoload compatibility layer, h/t @coenjacobs

There might be more options to explore here as well.

Progress

  • implement Composer in libraries used
  • add libraries to Packagist
  • require libraries in plugin
  • fix Composer installation to work
  • document
  • update build process (see #1961 )

Relevant pull requests

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jan 12, 2015

Contributor

@Rarst While from a developer's point of view, your intentions are understandable and good practice, let's not forget the reality.

The reality is that this is a WP plugin. My estimate is that 99% of WP users have never even heard of composer and that 99.9% have never used it.
After all, the average WP user is not a developer.

The most common way to install WP plugins is via the standard WP plugin install mechanism. The way the plugin is now set up with submodules, autoload working the way it does etc works in 100% of the cases for the normal WP plugin installs.

Do we really want to spend effort on something which might be used by 0.1% of the users - if at all ?

I'd suggest your time and effort is better placed in trying to get WP to up the mimimum requirement to PHP5.4 (or even better: 5.5) and to open up the discussion there of replacing the plugin install mechanism with a composer based system.

Anyways, that's my two pennies...

Contributor

jrfnl commented Jan 12, 2015

@Rarst While from a developer's point of view, your intentions are understandable and good practice, let's not forget the reality.

The reality is that this is a WP plugin. My estimate is that 99% of WP users have never even heard of composer and that 99.9% have never used it.
After all, the average WP user is not a developer.

The most common way to install WP plugins is via the standard WP plugin install mechanism. The way the plugin is now set up with submodules, autoload working the way it does etc works in 100% of the cases for the normal WP plugin installs.

Do we really want to spend effort on something which might be used by 0.1% of the users - if at all ?

I'd suggest your time and effort is better placed in trying to get WP to up the mimimum requirement to PHP5.4 (or even better: 5.5) and to open up the discussion there of replacing the plugin install mechanism with a composer based system.

Anyways, that's my two pennies...

@coenjacobs

This comment has been minimized.

Show comment
Hide comment
@coenjacobs

coenjacobs Jan 12, 2015

@jrfnl I think you misunderstand the purpose of Composer autoloading and how this gets deployed to the wordpress.org plugin repository. Composer autoloading generates files that can be included to add autoloading to your project. Developers of this project just need to include the Composer generated autoloading files to the package that can be downloaded (or updated) via the plugin repository. The end user doesn't even need to know Composer is there, let alone be able to use it.

@jrfnl I think you misunderstand the purpose of Composer autoloading and how this gets deployed to the wordpress.org plugin repository. Composer autoloading generates files that can be included to add autoloading to your project. Developers of this project just need to include the Composer generated autoloading files to the package that can be downloaded (or updated) via the plugin repository. The end user doesn't even need to know Composer is there, let alone be able to use it.

@Rarst

This comment has been minimized.

Show comment
Hide comment
@Rarst

Rarst Jan 12, 2015

Contributor

The most common way to install WP plugins is via the standard WP plugin install mechanism.

This will not in any way compromise the regular distribution and install channel. It wasn't even remotely considered. :) This is development experience improvement, nothing changes or get sacrificed for the users.

I wouldn't push for any change that would come with regression. The point is to improve.

I'd suggest your time and effort is better placed

My time and effort are perfectly placed here, since this is paid work for Yoast. :)

Contributor

Rarst commented Jan 12, 2015

The most common way to install WP plugins is via the standard WP plugin install mechanism.

This will not in any way compromise the regular distribution and install channel. It wasn't even remotely considered. :) This is development experience improvement, nothing changes or get sacrificed for the users.

I wouldn't push for any change that would come with regression. The point is to improve.

I'd suggest your time and effort is better placed

My time and effort are perfectly placed here, since this is paid work for Yoast. :)

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jan 12, 2015

Contributor

@Rarst Point taken.

@coenjacobs I probably do. So far my experiences with Composer have been limited as I quit trying after abysmal experiences with it on Windows. I have taken note of the notion that Windows support has improved over the last six months, but am very wary and sceptical after having been bitten before. That taken together with the x times duplicate code being littered all over the place, which still doesn't make any sense to me and the amount of config it takes to avoid that, gives me very little motivation to spend any time on giving it another try.

Contributor

jrfnl commented Jan 12, 2015

@Rarst Point taken.

@coenjacobs I probably do. So far my experiences with Composer have been limited as I quit trying after abysmal experiences with it on Windows. I have taken note of the notion that Windows support has improved over the last six months, but am very wary and sceptical after having been bitten before. That taken together with the x times duplicate code being littered all over the place, which still doesn't make any sense to me and the amount of config it takes to avoid that, gives me very little motivation to spend any time on giving it another try.

@Rarst

This comment has been minimized.

Show comment
Hide comment
@Rarst

Rarst Jan 12, 2015

Contributor

I quit trying after abysmal experiences with it on Windows. I have taken note of the notion that Windows support has improved over the last six months

Interesting point, since I am pretty much exclusively Windows user locally and hadn't had major issues in about two years of using it.

I will do my best on smooth and well documented experience here. :) You are welcome to contact me if you need any pointers in this context or about Composer in general.

Contributor

Rarst commented Jan 12, 2015

I quit trying after abysmal experiences with it on Windows. I have taken note of the notion that Windows support has improved over the last six months

Interesting point, since I am pretty much exclusively Windows user locally and hadn't had major issues in about two years of using it.

I will do my best on smooth and well documented experience here. :) You are welcome to contact me if you need any pointers in this context or about Composer in general.

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jan 12, 2015

Contributor

@Rarst I appreciate the offer and might take you up on it when I have some time to have another look at it. Thanks.

Contributor

jrfnl commented Jan 12, 2015

@Rarst I appreciate the offer and might take you up on it when I have some time to have another look at it. Thanks.

@Rarst

This comment has been minimized.

Show comment
Hide comment
@Rarst

Rarst Jan 13, 2015

Contributor

Looked into how composer-php52 works. Essentially it implements my idea from (2), making use of classmap already generated by Composer and wrapping it into 5.2–compatible loader implementation. Looks solid. Hadn't tested out in real 5.2 environment yet.

Contributor

Rarst commented Jan 13, 2015

Looked into how composer-php52 works. Essentially it implements my idea from (2), making use of classmap already generated by Composer and wrapping it into 5.2–compatible loader implementation. Looks solid. Hadn't tested out in real 5.2 environment yet.

@Rarst

This comment has been minimized.

Show comment
Hide comment
@Rarst

Rarst Jan 14, 2015

Contributor

Stalled on decision about retaining submodules to start on an implementing branch. In my opinion they should go, but that's from where I stand. cc @jdevalk

Contributor

Rarst commented Jan 14, 2015

Stalled on decision about retaining submodules to start on an implementing branch. In my opinion they should go, but that's from where I stand. cc @jdevalk

@jdevalk

This comment has been minimized.

Show comment
Hide comment
@jdevalk

jdevalk Jan 14, 2015

Member

Let's ditch 'em, but the team will need a proper explanation on how to do a new checkout :-)

Member

jdevalk commented Jan 14, 2015

Let's ditch 'em, but the team will need a proper explanation on how to do a new checkout :-)

@Rarst

This comment has been minimized.

Show comment
Hide comment
@Rarst

Rarst Jan 14, 2015

Contributor

Of course. :) Once I have working branch, I'll write up the documentation and work with everyone who needs it on the new process.

Contributor

Rarst commented Jan 14, 2015

Of course. :) Once I have working branch, I'll write up the documentation and work with everyone who needs it on the new process.

@jdevalk

This comment has been minimized.

Show comment
Hide comment
@jdevalk

jdevalk Jan 14, 2015

Member

👍

Member

jdevalk commented Jan 14, 2015

👍

@Rarst

This comment has been minimized.

Show comment
Hide comment
@Rarst

Rarst Jan 21, 2015

Contributor

First draft of documentation is up: Using Composer

Contributor

Rarst commented Jan 21, 2015

First draft of documentation is up: Using Composer

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jan 21, 2015

Contributor

@Rarst I had a quick look a the documentation. Just checking if I understand correctly: does this mean that each time you do a git checkout or switch branch, you need to run a command line command as well to verify the dependencies are still correct ?

Contributor

jrfnl commented Jan 21, 2015

@Rarst I had a quick look a the documentation. Just checking if I understand correctly: does this mean that each time you do a git checkout or switch branch, you need to run a command line command as well to verify the dependencies are still correct ?

@coenjacobs

This comment has been minimized.

Show comment
Hide comment
@coenjacobs

coenjacobs Jan 22, 2015

@jrfnl That's correct. composer install ensures you have the dependencies installed according to the composer.lock file (or composer.json if there is no .lock file). This file can change between commits and branches, so yeah you need to run the command at least after checkout and switching branches.

@jrfnl That's correct. composer install ensures you have the dependencies installed according to the composer.lock file (or composer.json if there is no .lock file). This file can change between commits and branches, so yeah you need to run the command at least after checkout and switching branches.

@jdevalk

This comment has been minimized.

Show comment
Hide comment
@jdevalk

jdevalk Jan 22, 2015

Member

Good thing there are post-checkout hooks in Git ;-)


Joost de Valk

If this email contains typo’s and is short, it might be sent from my mobile device. If it’s long that might still be true in which case I truly like you.

On Thu, Jan 22, 2015 at 7:43 AM, Coen Jacobs notifications@github.com
wrote:

@jrfnl That's correct. composer install ensures you have the dependencies installed according to the composer.lock file (or composer.json if there is no .lock file). This file can change between commits and branches, so yeah you need to run the command at least after checkout and switching branches.

Reply to this email directly or view it on GitHub:
#1890 (comment)

Member

jdevalk commented Jan 22, 2015

Good thing there are post-checkout hooks in Git ;-)


Joost de Valk

If this email contains typo’s and is short, it might be sent from my mobile device. If it’s long that might still be true in which case I truly like you.

On Thu, Jan 22, 2015 at 7:43 AM, Coen Jacobs notifications@github.com
wrote:

@jrfnl That's correct. composer install ensures you have the dependencies installed according to the composer.lock file (or composer.json if there is no .lock file). This file can change between commits and branches, so yeah you need to run the command at least after checkout and switching branches.

Reply to this email directly or view it on GitHub:
#1890 (comment)

@Rarst

This comment has been minimized.

Show comment
Hide comment
@Rarst

Rarst Jan 22, 2015

Contributor

Just checking if I understand correctly: does this mean that each time you do a git checkout or switch branch, you need to run a command line command as well to verify the dependencies are still correct ?

Yes, the state is now managed via Composer and captured in composer.lock. For the context it might seem like an “extra” move required, but it comes with number of serious advantages, most prominently:

  • easier and more reliable dependency updates, rather than micromanaging this by hand composer update will raise dependencies to latest available, while at the same time checking everything for version requirements and conflicts
  • Composer actually manages two states — production and development, you can choose between building the slim version or the one with any and all development/testing tools, using just one composer.json configuration (the WP SEO doesn't have any development dependencies configured yet, but it very well would make sense on later stages)
Contributor

Rarst commented Jan 22, 2015

Just checking if I understand correctly: does this mean that each time you do a git checkout or switch branch, you need to run a command line command as well to verify the dependencies are still correct ?

Yes, the state is now managed via Composer and captured in composer.lock. For the context it might seem like an “extra” move required, but it comes with number of serious advantages, most prominently:

  • easier and more reliable dependency updates, rather than micromanaging this by hand composer update will raise dependencies to latest available, while at the same time checking everything for version requirements and conflicts
  • Composer actually manages two states — production and development, you can choose between building the slim version or the one with any and all development/testing tools, using just one composer.json configuration (the WP SEO doesn't have any development dependencies configured yet, but it very well would make sense on later stages)
@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jan 22, 2015

Contributor

@Rarst @jdevalk Hope that someone wants to helps me set this up with git hooks then as otherwise I'll be out. There's now way that workflow (manually) will work for me. I use a Git Gui, so normally don't have command line open.

Contributor

jrfnl commented Jan 22, 2015

@Rarst @jdevalk Hope that someone wants to helps me set this up with git hooks then as otherwise I'll be out. There's now way that workflow (manually) will work for me. I use a Git Gui, so normally don't have command line open.

@Rarst

This comment has been minimized.

Show comment
Hide comment
@Rarst

Rarst Jan 22, 2015

Contributor

@jrfnl what is your editor of choice? Modern PHP editors/IDEs (I use PhpStorm) tend to have Composer support by now (so you can do Composer things without leaving editor / going to command line).

Contributor

Rarst commented Jan 22, 2015

@jrfnl what is your editor of choice? Modern PHP editors/IDEs (I use PhpStorm) tend to have Composer support by now (so you can do Composer things without leaving editor / going to command line).

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jan 22, 2015

Contributor

@Rarst I do most work in ConText which does not have Composer support. I only use PHPStorm occassionally as for dialy use it's way too resource heavy and I find the effective screen estate too small for big projects.

Contributor

jrfnl commented Jan 22, 2015

@Rarst I do most work in ConText which does not have Composer support. I only use PHPStorm occassionally as for dialy use it's way too resource heavy and I find the effective screen estate too small for big projects.

@Rarst

This comment has been minimized.

Show comment
Hide comment
@Rarst

Rarst Jan 22, 2015

Contributor

@jrfnl here is how you can set it up in ConText, as a suggestion:

  1. Options > Environment Options > Execute Keys
  2. Add > json
  3. F9
  4. Execute c:\ProgramData\ComposerSetup\bin\composer.bat (or respective path in your system)
  5. Start in %p
  6. Parameters install --no-interaction
  7. Capture console output > check
  8. Ok

Then opening any composer.json and F9 should run the install. :)

Contributor

Rarst commented Jan 22, 2015

@jrfnl here is how you can set it up in ConText, as a suggestion:

  1. Options > Environment Options > Execute Keys
  2. Add > json
  3. F9
  4. Execute c:\ProgramData\ComposerSetup\bin\composer.bat (or respective path in your system)
  5. Start in %p
  6. Parameters install --no-interaction
  7. Capture console output > check
  8. Ok

Then opening any composer.json and F9 should run the install. :)

@jdevalk

This comment has been minimized.

Show comment
Hide comment
@jdevalk

jdevalk Jan 22, 2015

Member

(heh @jrfnl, I should note, unlike all of us here at Yoast HQ, @Rarst is actually a Windows guy)

Member

jdevalk commented Jan 22, 2015

(heh @jrfnl, I should note, unlike all of us here at Yoast HQ, @Rarst is actually a Windows guy)

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jan 22, 2015

Contributor

@Rarst Wow! I appreciate you looking into this. All the same, a git hook makes more sense to me as having it coupled to Git would mean I don't have to think about it at all (except for copying the git hook in when first cloning a repo, but then again, I could just set it up as a default hook and don't even have to worry about that anymore).

@Rarst @jdevalk glad to hear there are more devs who don't abhor Windows ;-)

Contributor

jrfnl commented Jan 22, 2015

@Rarst Wow! I appreciate you looking into this. All the same, a git hook makes more sense to me as having it coupled to Git would mean I don't have to think about it at all (except for copying the git hook in when first cloning a repo, but then again, I could just set it up as a default hook and don't even have to worry about that anymore).

@Rarst @jdevalk glad to hear there are more devs who don't abhor Windows ;-)

@Rarst

This comment has been minimized.

Show comment
Hide comment
@Rarst

Rarst Jan 22, 2015

Contributor

I will look into Git hook approach too, but from quick search it seems that hooks aren't really “shared” via repository, so everyone could as well set it up in a way convenient to their tools of choice for starters.

Contributor

Rarst commented Jan 22, 2015

I will look into Git hook approach too, but from quick search it seems that hooks aren't really “shared” via repository, so everyone could as well set it up in a way convenient to their tools of choice for starters.

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jan 22, 2015

Contributor

@Rarst What do you mean by hooks aren't really “shared” via repository ? You can have repository hooks on the 'receiving end' - like the travis run -, and local hooks on the 'sending end', like running a php lint on each commit (yeah, I got that set up after it bit me once).
Local hooks aren't shared via the repo, but examples could be shared in the wiki or even in a subdirectory of the new 'bin' dir (see PR #1919) with a simple instruction in the wiki to just copy the hook to the .git/hooks/ directory for anyone who wants/needs it.
Though whether someone would need it would - as you rightfully say - depend on their own workflow and tools.

Contributor

jrfnl commented Jan 22, 2015

@Rarst What do you mean by hooks aren't really “shared” via repository ? You can have repository hooks on the 'receiving end' - like the travis run -, and local hooks on the 'sending end', like running a php lint on each commit (yeah, I got that set up after it bit me once).
Local hooks aren't shared via the repo, but examples could be shared in the wiki or even in a subdirectory of the new 'bin' dir (see PR #1919) with a simple instruction in the wiki to just copy the hook to the .git/hooks/ directory for anyone who wants/needs it.
Though whether someone would need it would - as you rightfully say - depend on their own workflow and tools.

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jan 22, 2015

Contributor

@Rarst There's quite some interesting examples floating around actually if you search Git/gists for git hooks or composer git hooks.
I'm not well versed enough in shell to figure out half of them. Took me nearly two days to get the lint set up and working 😊

Edit: this script is what I based my pre-commit php lint hook on: https://gist.github.com/manchuck/6538826

Contributor

jrfnl commented Jan 22, 2015

@Rarst There's quite some interesting examples floating around actually if you search Git/gists for git hooks or composer git hooks.
I'm not well versed enough in shell to figure out half of them. Took me nearly two days to get the lint set up and working 😊

Edit: this script is what I based my pre-commit php lint hook on: https://gist.github.com/manchuck/6538826

@Rarst

This comment has been minimized.

Show comment
Hide comment
@Rarst

Rarst Jan 22, 2015

Contributor

I was getting at that it's more of a local setup step, than something we can bolt into the project itself (as I understand).

Keep coming up on Bash specific examples of hooks, so not yet sure about Windows–friendliness of getting this working (I prefer vanilla cmd historically, your link is PowerShell, which I hadn't used much).

Contributor

Rarst commented Jan 22, 2015

I was getting at that it's more of a local setup step, than something we can bolt into the project itself (as I understand).

Keep coming up on Bash specific examples of hooks, so not yet sure about Windows–friendliness of getting this working (I prefer vanilla cmd historically, your link is PowerShell, which I hadn't used much).

@Rarst

This comment has been minimized.

Show comment
Hide comment
@Rarst

Rarst Jan 23, 2015

Contributor

I have tested the hook from this gist's comments (not the gist itself) and it works nicely for automated composer install when composer.lock changes on checkout.

@jrfnl if you could please try this one out on my fork with composer branch and tell if it works for you?

Contributor

Rarst commented Jan 23, 2015

I have tested the hook from this gist's comments (not the gist itself) and it works nicely for automated composer install when composer.lock changes on checkout.

@jrfnl if you could please try this one out on my fork with composer branch and tell if it works for you?

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jan 23, 2015

Contributor

@Rarst Will try but might be a little while as I haven't got composer set up at all yet

Contributor

jrfnl commented Jan 23, 2015

@Rarst Will try but might be a little while as I haven't got composer set up at all yet

@Rarst

This comment has been minimized.

Show comment
Hide comment
@Rarst

Rarst Jan 23, 2015

Contributor

They got super easy peasy installer for Win by now: https://getcomposer.org/Composer-Setup.exe

Contributor

Rarst commented Jan 23, 2015

They got super easy peasy installer for Win by now: https://getcomposer.org/Composer-Setup.exe

@Rarst

This comment has been minimized.

Show comment
Hide comment
@Rarst

Rarst Jan 29, 2015

Contributor

Aaand all merged! I'll run it some through PHP 5.2 and stuff locally.

What's the current release process and who do I need to work with to get that updated?

Anyone has outstanding issues with moving to Composer process in general?

Contributor

Rarst commented Jan 29, 2015

Aaand all merged! I'll run it some through PHP 5.2 and stuff locally.

What's the current release process and who do I need to work with to get that updated?

Anyone has outstanding issues with moving to Composer process in general?

@jdevalk

This comment has been minimized.

Show comment
Hide comment
@jdevalk

jdevalk Jan 29, 2015

Member

I currently get this:

[UnexpectedValueException]
  Could not parse version constraint ^1.0.16: Invalid version string "^1.0.16
  "

When I merge trunk into my pull #1923

Member

jdevalk commented Jan 29, 2015

I currently get this:

[UnexpectedValueException]
  Could not parse version constraint ^1.0.16: Invalid version string "^1.0.16
  "

When I merge trunk into my pull #1923

@Rarst

This comment has been minimized.

Show comment
Hide comment
@Rarst

Rarst Jan 29, 2015

Contributor

Version thing fixed with composer selfupdate, it's just a more recent feature.

Contributor

Rarst commented Jan 29, 2015

Version thing fixed with composer selfupdate, it's just a more recent feature.

@Rarst

This comment has been minimized.

Show comment
Hide comment
@Rarst

Rarst Jan 29, 2015

Contributor

Upstream issue, Composer change broke PHP 5.2 autoload generator with worst possible timing :(

See Issue #6

Contributor

Rarst commented Jan 29, 2015

Upstream issue, Composer change broke PHP 5.2 autoload generator with worst possible timing :(

See Issue #6

@Rarst

This comment has been minimized.

Show comment
Hide comment
@Rarst

Rarst Jan 29, 2015

Contributor

Workaround:

composer selfupdate 1.0.0-alpha9

and change to "xrstf/composer-php52": ">=1.0.16" in composer.json.

Contributor

Rarst commented Jan 29, 2015

Workaround:

composer selfupdate 1.0.0-alpha9

and change to "xrstf/composer-php52": ">=1.0.16" in composer.json.

@jdevalk

This comment has been minimized.

Show comment
Hide comment
@jdevalk

jdevalk Jan 30, 2015

Member

That works. As for build, I'll share our current release script with you and discuss.

Member

jdevalk commented Jan 30, 2015

That works. As for build, I'll share our current release script with you and discuss.

@Rarst

This comment has been minimized.

Show comment
Hide comment
@Rarst

Rarst Feb 13, 2015

Contributor

Short of deploy, for which there is working update to current process, we are good here. Composer all the things! :)

Contributor

Rarst commented Feb 13, 2015

Short of deploy, for which there is working update to current process, we are good here. Composer all the things! :)

@Rarst Rarst closed this Feb 13, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment