Skip to content
This repository has been archived by the owner on Nov 10, 2022. It is now read-only.

Recommend Composer or NPM instead of Git submodules #278

Merged
merged 111 commits into from
Jan 17, 2019

Conversation

kasparsd
Copy link
Contributor

@kasparsd kasparsd commented Apr 20, 2018

  • Update readme to recommend installing wp-dev-lib via Composer or npm.
  • Update scripts to match the new location in the scripts directory.
  • Update generate-markdown-readme logic to work with the new layout. Make sure the readme.md never gets out of sync with the readme.txt, if it is staged for commit.
  • Update Git submodule instructions.
  • Newly added tests are failing on Travis with PHP Warning: mysqli_query(): Couldn't fetch mysqli in wp-includes/wp-db.php.
  • Start tagging releases to allow installing specific versions of the package, see Start versioning wp-dev-lib #277.

New features:

  • Added integration testing for the library being used with Composer. Travis will run the actual pre-commit logic for a test project under /tests/composer. Since the pre-commit logic relies on a Git repository at the project root, we create a fresh repo at /tests/composer, commit phpcs.xml to master and then commit the rest of the files to a test branch which is used for the pre-commit checks.

@westonruter
Copy link
Contributor

This would fix #83?

@kasparsd
Copy link
Contributor Author

@westonruter Yes, it would fix also #83.

Are you OK with the general idea of recommending Composer and npm install instead of the git submodules?

@westonruter
Copy link
Contributor

Yes, I'm probably just old fashioned for still clinging to what is familiar to me.

install-pre-commit-hook.sh Outdated Show resolved Hide resolved
fi

if [ ! -e "$DEV_LIB_PATH/check-diff.sh" ]; then
echo "Unable to determine DEV_LIB_PATH"

exec < /dev/tty # to be able to use read
read -p "Would you like to clone wp-dev-lib to a temporary dir? [y/N] " -r -n 1
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now require this to be a proper project dependancy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this another change that should be rolled back since submodules are still ok if desired?

scripts/pre-commit Outdated Show resolved Hide resolved
@kasparsd
Copy link
Contributor Author

kasparsd commented Jun 2, 2018

OK, this is super strange. It is actually failing during WP_UnitTestCase::tearDownAfterClass() when it calls _delete_all_data():

function _delete_all_data() {
	global $wpdb;

	foreach ( array(
		$wpdb->posts,
		$wpdb->postmeta,
		$wpdb->comments,
		$wpdb->commentmeta,
		$wpdb->term_relationships,
		$wpdb->termmeta,
	) as $table ) {
		$wpdb->query( "DELETE FROM {$table}" );
	}

	foreach ( array(
		$wpdb->terms,
		$wpdb->term_taxonomy,
	) as $table ) {
		$wpdb->query( "DELETE FROM {$table} WHERE term_id != 1" );
	}

	$wpdb->query( "UPDATE {$wpdb->term_taxonomy} SET count = 0" );

	$wpdb->query( "DELETE FROM {$wpdb->users} WHERE ID != 1" );
	$wpdb->query( "DELETE FROM {$wpdb->usermeta} WHERE user_id != 1" );
}

while doing any queries during the actual tests works just fine.

@valendesigns
Copy link
Contributor

@kasparsd How close is this to being done? I'm working on an updated version of the wp-foo-bar plugin that is using this branch and I'd like to merge this PR before merging the other. What is outstanding or needs review?

@kasparsd
Copy link
Contributor Author

kasparsd commented Oct 6, 2018

@valendesigns I think it is pretty much done. Will look into resolving the merge conflicts.

Could you please check the updated readme copy and ensure it sounds good and clear?

@kasparsd kasparsd self-assigned this Jan 16, 2019
@kasparsd
Copy link
Contributor Author

@westonruter This is ready for another round of code review.

I've tagged a 0.1.0 release to ensure users can always revert to the previous version.

"test": [
"find ./scripts -name *.php -print0 | xargs -0 -n1 -P8 php -l",
"shellcheck **/*.sh || true",
"shellcheck --format json **/*.sh | grep --quiet --invert-match '\"level\":\"error\"'"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fail only on errors for now.

@westonruter
Copy link
Contributor

It's working well in my testing of the composer method: ampproject/amp-wp#1131

@westonruter westonruter merged commit 90574b4 into master Jan 17, 2019
@westonruter
Copy link
Contributor

@kasparsd would you tag a new release? I'm guessing that would be 0.2.0? We'll then use that in the AMP plugin.

@westonruter westonruter deleted the feature/composer-first branch January 17, 2019 05:01
@kasparsd
Copy link
Contributor Author

@westonruter I tagged it as 1.0.0 as this is a breaking change since files have been moved around.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants