MigrationRunner::version should return "current version string on success"

[Martin-4Spaces]

According to the function declaration for MigrationRunner::version, the function should return

mixed TRUE if no migrations are found, current version string on success, FALSE on failure

It does not return "current version string on success". It will always return true on success.

Here is a coy of the function from current developer branch.

    public function version(string $targetVersion, string $namespace = null, string $group = null)
	{
		if (! $this->enabled)
		{
			throw ConfigException::forDisabledMigrations();
		}

		$this->ensureTable();

		// Set Namespace if not null
		if (! is_null($namespace))
		{
			$this->setNamespace($namespace);
		}

		// Set database group if not null
		if (! is_null($group))
		{
			$this->setGroup($group);
		}

		// Sequential versions need adjusting to 3 places so they can be found later.
		if ($this->type === 'sequential')
		{
			$targetVersion = str_pad($targetVersion, 3, '0', STR_PAD_LEFT);
		}

		$migrations = $this->findMigrations();

		if (empty($migrations))
		{
			return true;
		}

		// Get Namespace current version
		// Note: We use strings, so that timestamp versions work on 32-bit systems
		$currentVersion = $this->getVersion();
		if ($targetVersion > $currentVersion)
		{
			// Moving Up
			$method = 'up';
			ksort($migrations);
		}
		else
		{
			// Moving Down, apply in reverse order
			$method = 'down';
			krsort($migrations);
		}

		// Check Migration consistency
		$this->checkMigrations($migrations, $method, $targetVersion);

		// loop migration for each namespace (module)
		foreach ($migrations as $version => $migration)
		{
			// Only include migrations within the scoop
			if (($method === 'up' && $version > $currentVersion && $version <= $targetVersion) || ( $method === 'down' && $version <= $currentVersion && $version > $targetVersion)
			)
			{
				include_once $migration->path;
				// Get namespaced class name
				$class = $this->namespace . '\Database\Migrations\Migration_' . ($migration->name);

				$this->setName($migration->name);

				// Validate the migration file structure
				if (! class_exists($class, false))
				{
					throw new \RuntimeException(sprintf(lang('Migrations.classNotFound'), $class));
				}

				// Forcing migration to selected database group
				$instance = new $class(\Config\Database::forge($this->group));

				if (! is_callable([$instance, $method]))
				{
					throw new \RuntimeException(sprintf(lang('Migrations.missingMethod'), $method));
				}

				$instance->{$method}();
				if ($method === 'up')
				{
					$this->addHistory($migration->version);
				}
				elseif ($method === 'down')
				{
					$this->removeHistory($migration->version);
				}
			}
		}

		return true;
	}

[atishhamte]

@jim-parry @lonnieezell , I guess, the better return type of the function should be,
Current version string on success or False if no migrations to perform or failure in process.

[lonnieezell]

@atishamte I agree with that. I guess this is why you submitted the other PR. I had not seen this comment until I had already looked at the PR and was confused :) Next time maybe include the issue # in the description so we know what it's solving.