Problem/Motivation

The core/.gitignore file contains directives only for the vendor/ directory. Since we moved out vendor to the root, there is no reason for this file. Let's remove it.

Proposed resolution

Since core/.gitignore contains important policies about the D8 repo, we should move that behavior to our composer scripts or plugins, and/or packaging scripts.

And remove core/.gitignore.

Remaining tasks

Patch

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#10 remove_core_gitignore-2635584-10.patch833.27 KBhussainweb
#5 remove_core_gitignore-2635584-5.patch1.07 MBhussainweb
#2 remove_core_gitignore-2635584-2.patch1.28 KBhussainweb
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hussainweb created an issue. See original summary.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented
Issue summary: View changes
Status: Active » Needs review
FileSize
remove_core_gitignore-2635584-2.patch1.28 KB

Here is the patch.

Crell’s picture

Crell CreditAttribution: Crell at Palantir.net commented
Status: Needs review » Reviewed & tested by the community

Seems legit. Thanks.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work

It does look like we need to get rid of this but we need to do a bit more...

  1. +++ /dev/null
@@ -1,19 +0,0 @@
-# SimpleTest breaks with the following files, so avoid adding them.
-vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services1-1.php
-vendor/symfony/class-loader/Symfony/Component/ClassLoader/Tests/Fixtures/php5.4/traits.php
-vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services11.php

    Tests are being removed and this hasn't worked for ages. 

  2. +++ /dev/null
@@ -1,19 +0,0 @@
-# The resources for the Validator component are not required.
-vendor/symfony/validator/Symfony/Component/Validator/Resources

    This issue should add this to Drupal\Core\Composer\Composer::$packageToCleanup and do the remove. 

  3. +++ /dev/null
@@ -1,19 +0,0 @@
-# Symfony Validator depends on Symfony Translation but only requires the
-# TranslatorInterface. Thus, we add only the required interface from Symfony
-# Translation by ignoring everything except the interface.
-vendor/symfony/translation/Symfony/Component/Translation/*
-!vendor/symfony/translation/Symfony/Component/Translation/TranslatorInterface.php

    So this was broken again... I can't remember what we decided last time. 

  4. +++ /dev/null
@@ -1,19 +0,0 @@
-# PHPUnit provides some binary dependencies that are already available.
-vendor/phpunit/phpunit/build/dependencies

    Doesn't exist anymore. 

  5. +++ /dev/null
@@ -1,19 +0,0 @@
-# The PHAR file below contains CRLF characters that cause a problem with PIFR.
-vendor/symfony/dependency-injection/Tests/Fixtures/includes/ProjectWithXsdExtensionInPhar.phar

    Removed by Drupal\Core\Composer\Composer so all good here...

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented
Status: Needs work » Needs review
FileSize
remove_core_gitignore-2635584-5.patch1.07 MB

It seems like the only points that need work are 2 and 3. I have added Resources to the $packageToCleanup but not yet figured out how to handle the other one. Anyway, I am not convinced we should do this at all. I have attached the patch with changes anyway as I am curious.

Imagine a scenario where these components are actually used by other modules or other components. I am not sure how any other component can override core's composer.json to not run the cleanup in that case. I guess it's okay if we are not keeping tests but Resources and other files seem to be required.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs review » Needs work

@hussainweb - yeah I wasn't sure what we decided for #3 I think that it is best to leave this alone for now.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

By leave it alone - what I mean let's not remove any of the Translation component because we have no way of maintaining that.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented

@alexpott: What about Resources in the validator component? The same argument applies. What if another module needed that?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@hussainweb I guess but that one is trickier because they are translations that just can not play nice with Drupal's translation system

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented
Status: Needs work » Needs review
FileSize
remove_core_gitignore-2635584-10.patch833.27 KB

For review, here is a patch with just the Resources removed as discussed in previous comments.

I understand these translations cannot work with the Drupal's translation system, but is there any harm in letting them stay. They were in the repository so far and didn't cause any issues. We are going to remove all third party components soon from the repository anyway, but this change will remove it every time composer install runs as well.

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented
Parent issue: » #1475510: Remove external dependencies from the core repo and let Composer manage the dependencies instead

Adding #1475510: Remove external dependencies from the core repo and let Composer manage the dependencies instead as parent issue, this originated from there and is blocking it.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Title: Remove core/.gitignore file » Move core/.gitignore policies to composer events/packaging scripts
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

We're about to remove vendor/ so we don't really need to remove any vendor files here, but it's not a big deal if we do.

Updated title and issue status.

Everything in #4 is satisfied including #4.3, because we won't manage it according to #4.7.

Unfortunately we can't say composer install --no-script in order to compare the behavior of the changed composer script, because then it won't run the wikimedia plugin. If anyone has any magic incantation to prove it works, give it a try.

Otherwise: RTBC. Woot!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed 772127c and pushed to 8.1.x. Thanks!

  • alexpott committed 772127c on 8.1.x 
    Issue #2635584 by hussainweb, alexpott: Move core/.gitignore policies to...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.